Invert default visibility #347
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I am still uncertain if hide_category or something like hide_by_default. "Hide_category = true" sounds good to me. I felt like adding the word default is unnecessary. On the other hand, the current behavior of Burrito is to hide all categories by default so we may want to change this behavior in the future. |
Xml converter
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Full Diff
--- xml_converter/integration_tests/test_cases/default_toggle/output_proto/markers.bin.textproto._old 2024-09-05 01:20:42.741849646 +0000
+++ xml_converter/integration_tests/test_cases/default_toggle/output_proto/markers.bin.textproto._new 2024-09-05 01:20:42.749849660 +0000
@@ -0,0 +1,50 @@
+category {
+ name: "My Category"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\350\314\006\302\223^\226"
+}
+category {
+ name: "My Category 2"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3404\0141b"
+}
+category {
+ name: "My Category 3"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\355i\3405\0141b"
+}
+category {
+ name: "My Category 4"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3406\0141b"
+}
There was a problem hiding this comment.
Full Diff
--- xml_converter/integration_tests/test_cases/proto_hide_category/input/pack/markers.bin.textproto._old 2024-09-05 01:20:42.761849680 +0000
+++ xml_converter/integration_tests/test_cases/proto_hide_category/input/pack/markers.bin.textproto._new 2024-09-05 01:20:42.765849687 +0000
@@ -0,0 +1,50 @@
+category {
+ name: "My Category"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\350\314\006\302\223^\226"
+}
+category {
+ name: "My Category 2"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3404\0141b"
+}
+category {
+ name: "My Category 3"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\355i\3405\0141b"
+}
+category {
+ name: "My Category 4"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3406\0141b"
+}
There was a problem hiding this comment.
Full Diff
--- xml_converter/integration_tests/test_cases/proto_hide_category/output_proto/markers.bin.textproto._old 2024-09-05 01:20:42.773849700 +0000
+++ xml_converter/integration_tests/test_cases/proto_hide_category/output_proto/markers.bin.textproto._new 2024-09-05 01:20:42.781849713 +0000
@@ -0,0 +1,50 @@
+category {
+ name: "My Category"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\350\314\006\302\223^\226"
+}
+category {
+ name: "My Category 2"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3404\0141b"
+}
+category {
+ name: "My Category 3"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\355i\3405\0141b"
+}
+category {
+ name: "My Category 4"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3406\0141b"
+}
There was a problem hiding this comment.
Full Diff
--- xml_converter/integration_tests/test_cases/proto_hide_category/input/pack/markers.bin.textproto._old 2024-09-05 01:23:50.377220259 +0000
+++ xml_converter/integration_tests/test_cases/proto_hide_category/input/pack/markers.bin.textproto._new 2024-09-05 01:23:50.385220281 +0000
@@ -0,0 +1,50 @@
+category {
+ name: "My Category"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\350\314\006\302\223^\226"
+}
+category {
+ name: "My Category 2"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3404\0141b"
+}
+category {
+ name: "My Category 3"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\355i\3405\0141b"
+}
+category {
+ name: "My Category 4"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3406\0141b"
+}
There was a problem hiding this comment.
Full Diff
--- xml_converter/integration_tests/test_cases/proto_hide_category/output_proto/markers.bin.textproto._old 2024-09-05 01:23:50.389220292 +0000
+++ xml_converter/integration_tests/test_cases/proto_hide_category/output_proto/markers.bin.textproto._new 2024-09-05 01:23:50.397220314 +0000
@@ -0,0 +1,50 @@
+category {
+ name: "My Category"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\350\314\006\302\223^\226"
+}
+category {
+ name: "My Category 2"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3404\0141b"
+}
+category {
+ name: "My Category 3"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\355i\3405\0141b"
+}
+category {
+ name: "My Category 4"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3406\0141b"
+}
There was a problem hiding this comment.
Full Diff
--- xml_converter/integration_tests/test_cases/xml_default_toggle/output_proto/markers.bin.textproto._old 2024-09-05 01:23:50.405220336 +0000
+++ xml_converter/integration_tests/test_cases/xml_default_toggle/output_proto/markers.bin.textproto._new 2024-09-05 01:23:50.413220357 +0000
@@ -0,0 +1,50 @@
+category {
+ name: "My Category"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\350\314\006\302\223^\226"
+}
+category {
+ name: "My Category 2"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3404\0141b"
+}
+category {
+ name: "My Category 3"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\355i\3405\0141b"
+}
+category {
+ name: "My Category 4"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3406\0141b"
+}
AsherGlick
reviewed
Sep 5, 2024
xml_converter/proto/waypoint.proto
Outdated
| @@ -21,7 +21,7 @@ message Category { | |||
| repeated Icon icon = 3; | |||
| repeated Trail trail = 4; | |||
| bool is_separator = 5; | |||
| bool default_visibility = 6; | |||
| bool hide_category = 6; | |||
There was a problem hiding this comment.
What were other names that were considered here and why did you decide on hide_category?
As it is written now this field is Category.hide_category which seems redundant.
Just removing category and making it Category.hide might look like a good idea initially but will it make intuitive sense when combined with the hidden/shown safe file data too?
There was a problem hiding this comment.
The other top names I considered were hide_by_default and default_hide_category.
There is another field in the XML called hide that we made trigger.action_hide_category so I used that naming convention. Though on further thought, that field is a Category object and this one is a bool so maybe they don't need to be similar.
I considered using hide but was concerned that it was too similar to the Godot function hide() and that when accessing this field, Category.get_hide() isn't descriptive enough.
There was a problem hiding this comment.
My gut tells me that hide_category is too imperative / verb-ish when it should be more declarative / noun-ish.
There was a problem hiding this comment.
I agree
How about is_hidden? This makes it clear its a property not a command. Also fits with some other attributes like is_separator and is_wall.
There was a problem hiding this comment.
Full Diff
--- xml_converter/integration_tests/test_cases/proto_hide_category/input/pack/markers.bin.textproto._old 2024-09-08 00:48:04.767466414 +0000
+++ xml_converter/integration_tests/test_cases/proto_hide_category/input/pack/markers.bin.textproto._new 2024-09-08 00:48:04.775466387 +0000
@@ -0,0 +1,50 @@
+category {
+ name: "My Category"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\350\314\006\302\223^\226"
+}
+category {
+ name: "My Category 2"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3404\0141b"
+}
+category {
+ name: "My Category 3"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\355i\3405\0141b"
+}
+category {
+ name: "My Category 4"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3406\0141b"
+}
There was a problem hiding this comment.
Full Diff
--- xml_converter/integration_tests/test_cases/proto_hide_category/output_proto/markers.bin.textproto._old 2024-09-08 00:48:04.795466321 +0000
+++ xml_converter/integration_tests/test_cases/proto_hide_category/output_proto/markers.bin.textproto._new 2024-09-08 00:48:04.803466295 +0000
@@ -0,0 +1,50 @@
+category {
+ name: "My Category"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\350\314\006\302\223^\226"
+}
+category {
+ name: "My Category 2"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3404\0141b"
+}
+category {
+ name: "My Category 3"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\355i\3405\0141b"
+}
+category {
+ name: "My Category 4"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3406\0141b"
+}
There was a problem hiding this comment.
Full Diff
--- xml_converter/integration_tests/test_cases/xml_default_toggle/output_proto/markers.bin.textproto._old 2024-09-08 00:48:04.807466282 +0000
+++ xml_converter/integration_tests/test_cases/xml_default_toggle/output_proto/markers.bin.textproto._new 2024-09-08 00:48:04.815466255 +0000
@@ -0,0 +1,50 @@
+category {
+ name: "My Category"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\350\314\006\302\223^\226"
+}
+category {
+ name: "My Category 2"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3404\0141b"
+}
+category {
+ name: "My Category 3"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ default_visibility: true
+ id: "(\355i\3405\0141b"
+}
+category {
+ name: "My Category 4"
+ icon {
+ map_id: 50
+ position {
+ x: 169.81
+ y: 210.65
+ z: 215.83
+ }
+ }
+ id: "(\355i\3406\0141b"
+}
21 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changed Default_toggle to hide_category in the proto so the default can be 0. #151