feat: field template manager#1374
Conversation
TrigamDev
added
Type: Enhancement
CyanVoxel
added
Priority: Medium
CyanVoxel
left a comment
CyanVoxel
left a comment
There was a problem hiding this comment.
Thank you for your work on this so far!
Besides my comments here, you've seen from the Discord that I have great concerns about our current MVC recommendation that encourages controller classes to inherit from views now that we're moving beyond simple use cases. Now that you're doing the (correct) thing and looking to reduce code duplication with our old widgets, its exposed the issues with doing this from an inheritance perspective.
To briefly reiterate and to have a record here, the following type of class structure caused by our current MVC implementation recommendation:
class FieldTemplateSearchPanel(SearchPanel[BaseFieldTemplate], FieldTemplateSearchPanelView):
...Causes a tangled tree of unsafe and duplicated multiple inheritance:
FieldTemplateSearchPanel(C) <- SearchPanel(C) <- SearchPanelView(V) <- PanelWidget(V) <- QWidget
^- FieldTemplateSearchPanelView(V) <- SearchPanelView(V) <- PanelWidget(V) <- QWidget
While a more traditional approach of composing the view and controller together would eliminate all inheritance issues and code reuse hurdles while not sacrificing any functionality:
class FieldTemplateSearchPanel(): # Controller
def __init__(self, view=FieldTemplateSearchPanelView):
...FieldTemplateSearchPanelView(V) <- SearchPanelView(V) <- PanelWidget(V) <- QWidget
FieldTemplateSearchPanel(C) <- SearchPanel©
Passing a view to a controller's constructor and keeping a reference when needed should retain the functionality and simplicity afforded by our current recommendation but without any of the inheritance issues brought to light here.
Frankly I don't have too much of a preference as to whether or not we have the view composed in the controller or the controller composed in the view, I just know that the examples I've found do the later and it's closer to what we currently recommend. I just don't want to be caught several months into another UI refactor just to reach a point where we realize we need to refactor again due to an unexpected roadblock with our approach. If you have specific concerns about that approach or see benefits from the inverse, please let me know here.
I'd just hold off on doing a new refactor until a consensus is reached between us and @Computerdores on how to implement MVC moving forward, but it's something I do anticipate on changing in the near future. In the meantime I'm open to at least pulling this PR as it seems to at least function for the time being, and is required to unblock #1358 and #1359 for the upcoming 9.6.0 update.
| def __init__( | ||
| self, | ||
| library: Library, | ||
| exclude: Sequence[BaseFieldTemplate] | None = None, |
There was a problem hiding this comment.
Unlike tags, fields don't have the assumption that there should only be one of each at most on an entry, so it shouldn't be necessary to have exclusion logic for them
| exclude: Sequence[BaseFieldTemplate] | None = None, | ||
| is_field_template_chooser: bool = True, | ||
| ) -> None: | ||
| super().__init__([], is_field_template_chooser) |
There was a problem hiding this comment.
I would recommend explicitly calling the base classes' __init__() methods rather than using super() when using multiple inheritance for the reasons described here
| super().__init__([], is_field_template_chooser) | |
| SearchPanel.__init__(self, [], is_field_template_chooser) | |
| FieldTemplateSearchPanelView.__init__(self, is_field_template_chooser) |
There was a problem hiding this comment.
Explicitly calling the base classes' __init__() results in SearchPanelView being initialized twice and triggering a crash
There was a problem hiding this comment.
Dang, I must've forgotten about that when I was originally reviewing this a few days ago. I think the basedpyright error led me to this and then that crash led me to look into the init calls further and discover the issues with the inheritance tree.
Disregard these recommendations then, but the underlying issue with the super() call and inheritance tree is still present.
| [tool.pytest.ini_options] | ||
| #addopts = "-m 'not qt'" | ||
| qt_api = "pyside6" | ||
| pythonpath = ["src"] |
There was a problem hiding this comment.
I was having some issues with pytest not recognizing new files, and adding this line fixed it.
There was a problem hiding this comment.
Note: I've noticed that whatever sorting method you (and Weblate) are using for the translation keys is more accurate than the built-in "Sort Lines Ascending" function in VS Code for me. What are you using to sort these?
There was a problem hiding this comment.
2 participants
Summary
SearchPanelandSearchPanelView). The newFieldTemplateSearchPanelandFieldTemplateSearchPanelViewextend from these new classes.tag.translation keys that the tag search panel uses, thelibrary.field.translation keys have been changed tofield..field.translation keys have been added to be used by the field template search panel.enhas been correctly sorted (I ran a JSON sorter on it to make sure the new keys were correctly sorted, and found that existing keys weren't correctly sorted).Note
This pull request does NOT implement creating, editing, or deleting field templates. Some inputs, such as "Create" buttons, are visible, but do nothing when interacted with.
Tasks Completed