Fix Remove lambdas when updating menus from context in Window#6740
Fix Remove lambdas when updating menus from context in Window#6740DragaDoncila merged 6 commits intonapari:mainfrom
Window#6740Conversation
|
ping @Czaki @DragaDoncila |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6740 +/- ##
==========================================
- Coverage 92.39% 92.34% -0.06%
==========================================
Files 615 615
Lines 54854 54861 +7
==========================================
- Hits 50684 50660 -24
- Misses 4170 4201 +31 ☔ View full report in Codecov by Sentry. |
|
Hmm yeah @lucyleeow I can see what you mean about the repetition, and I don't love it. I don't personally see much issue with using the strings... even if we make them constants at the top of the file or something. Could we instead use partials to parameterize the update method with the right menu name before connecting it? |
|
Yeah I think that is what we wanted originally and I am happy to change to that! |
|
not sure if @Czaki has a preference |
| def _update_plugin_menu_state(self): | ||
| self._update_menu_state('plugins_menu') |
There was a problem hiding this comment.
I think @Czaki said string is fine as long as we type via
Literal.
Yes. Because most often we use in codebase:
def fun(label: str):
if label == "aa":
# something
elif label == "ab":
# something
elif label == "ac":
# something
else:
# somethingWhen allowed values are "aa", "ab", "ac", "ad". But the above code will also work for "ae". There will be no error for such typo. Using literal allow to detect such error using mypy at least for part of cases (also IDE my highlight).
Another option is to use Enum to enforce consistency. StrEnum may allow having runtime exception for inproper values.
So partial for each mene or defining a method for each update function? @Czaki ?
Separate method. Partial connected to signal will keep hard reference to the object and keep it alive when it should be deleted.
|
Okay I have used Let me know if this is okay with you, thanks! |
DragaDoncila
left a comment
DragaDoncila
left a comment
There was a problem hiding this comment.
LGTM thanks @lucyleeow
|
ping @Czaki 😬 |
lucyleeow
added
the
ready to merge
DragaDoncila
removed
the
ready to merge
3 participants
References and relevant issues
Follow on from #4991
Description
Removes lambdas when updating from context during
add_menu_qt_main_window.py.Each menu function just gets layerlist and does not use a helper function anymore. Prevents any use of strings now. Not 100% this is better. There is a lot of repetition but there was previously as well. Open to change.