Annotations: Use flag to determine if the layer is a Ruler
ClosedPublic
Actions

Authored by Antonio Vazquez (antoniov) on Oct 9 2019, 4:02 PM.

Details

Reviewers

Dalai Felinto (dfelinto)
Campbell Barton (campbellbarton)

Commits

rBea1174bde9ed: Annotations: Use flag to determine if the layer is a Ruler

Summary

Proposed fix for T70141.

Before, the ruler was using the name of the layer as key, but this is very weak because if the layer name changes, the layer gets an annotation layer.

Now, the layer is marked using a flag and now it's possible to rename it.

Diff Detail

Repository

rB Blender

Branch

temp-ruler (branched from master)

Build Status

Buildable 5297
Build 5297: arc lint + arc unit

Event Timeline

Antonio Vazquez (antoniov) created this revision.Oct 9 2019, 4:02 PM

Harbormaster completed remote builds in B5297: Diff 18898.Oct 9 2019, 4:02 PM

Antonio Vazquez (antoniov) mentioned this in T70141: Renaming Annotation Layer "RulerData3D" deletes the Ruler data normal?.Oct 9 2019, 4:03 PM

Antonio Vazquez (antoniov) added reviewers: Dalai Felinto (dfelinto), Campbell Barton (campbellbarton).

We need a doversion code for this as well, otherwise what happens with old files?

source/blender/editors/space_view3d/view3d_gizmo_ruler.c
411	Nitpick: - view3d_ruler_get_layer + view3d_ruler_layer_get We tend to have get/set in the end to group functions together with only their suffix changing and to help IDEs autocomplete. Even if it "sounds wrong" from the English point of view.
448	No need to spam flag setting into multiple lines, and your comment really is redundant. A good comment would focus more on the "why" not so much on the "what'. In this case you covered the "what" but it is really redundant, the one line of code says the same thing here. My suggestion: - gpl->flag \|= GP_LAYER_HIDE; - /* Enable ruler flag. */ - gpl->flag \|= GP_LAYER_IS_RULER; + gpl->flag \|= GP_LAYER_HIDE \| GP_LAYER_IS_RULER;`
source/blender/makesrna/intern/rna_gpencil.c
1461	Shouldn't this be readonly? Nitpick, what about `is_ruler` instead?

This revision now requires changes to proceed.Oct 9 2019, 5:16 PM

Annotations: Fix review problems

Harbormaster completed remote builds in B5302: Diff 18905.Oct 9 2019, 5:42 PM

Antonio Vazquez (antoniov) added inline comments.Oct 9 2019, 5:44 PM

source/blender/blenloader/intern/versioning_280.c
1273 ↗	(On Diff #18905)	Not sure if use this define. I could add the string directly, but as this name is used in other areas I wanted to keep the define to make easier if we need to find where the ruler name is used. We could move the define into shared place, but not sure where and/or if it worth it.

Some extra suggested changes, but if they get addressed, LGTM

source/blender/blenloader/intern/versioning_280.c
1272 ↗	(On Diff #18905)	No need to mention the task, git blame will give that already.
1273 ↗	(On Diff #18905)	doversion can be its own mad house, no need to elegant, shared, ... Honestly, I would probably just add it to the test itself: - if (STREQ(gpl->info, ruler_name)) { + if (STREQ(gpl->info, "RulerData3D")) {
1277 ↗	(On Diff #18905)	Alternative, to avoid extra nesting by continuing the for loop earlier. if (gpd == NULL) { continue; }
1280 ↗	(On Diff #18905)	Am I right that can be only one ruler layer? If so I would expect a `break;` after we found a match.
source/blender/makesrna/intern/rna_gpencil.c
1463	While we follow "is_*" for rna names, the UI text should be "Ruler" (also change on tooltip): - RNA_def_property_ui_text(prop, "Is Ruler", "This layer is a special ruler layer"); + RNA_def_property_ui_text(prop, "Ruler", "This is a special ruler layer");