feat: field template manager

[TrigamDev]

Summary

• Replaces the field template chooser with one similar to the tag chooser, and adds a field template manager modal.
  • To do this, the tag chooser and tag managers have been merged, split into a view and controller, and had most functionality made generic (creating a SearchPanel and SearchPanelView). The new FieldTemplateSearchPanel and FieldTemplateSearchPanelView extend from these new classes.
• To better match the tag. translation keys that the tag search panel uses, the library.field. translation keys have been changed to field..
• Several new field. translation keys have been added to be used by the field template search panel.
• en has 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

• Platforms Tested:
  • Windows x86
  • Windows ARM
  • macOS x86
  • macOS ARM
  • Linux x86
  • Linux ARM
• Tested For:
  • Basic functionality
  • PyInstaller executable

[CyanVoxel]

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.

[CyanVoxel]

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

[CyanVoxel]

I would recommend explicitly calling the base classes' __init__() methods rather than using super() when using multiple inheritance for the reasons described here

Suggested change

	super().__init__([], is_field_template_chooser)
	SearchPanel.__init__(self, [], is_field_template_chooser)
	FieldTemplateSearchPanelView.__init__(self, is_field_template_chooser)

[TrigamDev]

Explicitly calling the base classes' __init__() results in SearchPanelView being initialized twice and triggering a crash

[CyanVoxel]

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.

[CyanVoxel]

What's the purpose of this?

[TrigamDev]

I was having some issues with pytest not recognizing new files, and adding this line fixed it.

[CyanVoxel]

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?

[TrigamDev]

I'm using https://plugins.jetbrains.com/plugin/11104-json-sorter

[Computerdores]

just btw: iirc it's recommended to use MyClass[T]: instead of MyClass(Generic[T]) (see PEP 695)

Edit: This is relevant to multiple places in this PR and yes, the rest of the code base doesn't adhere to this either, but it would be good to do better into the future.

[Computerdores]

Ok I am a bit confused about why the inheritance was done the way it was in this PR, especially concerning the conclusion that our guidelines cause this, since they are not being followed here.

The intention behind our guidelines is that any new widget will be split into controller and view (in order to seperate app logic and UI), while still having a single interface, such that other code that is using the widget can't tell that the separation exists. This also means that other code should only ever reference the controller (which is why it should be called MyWidget not MyWidgetController), unless a view is reused in another widget - in which case there should be two controllers inheriting from one view (because the UI logic is the same, while the app logic is different).

With this in mind I would have expected a structure like this, where the specialised search panel makes use of a generic search panel with each being split into view and controller:

SpecialSearchPanel
        ↓
SpecialSearchPanelView
        ↓
SearchPanel[T]
        ↓
SearchPanelView

Instead this structure was used:

       SpecialSearchPanel
         ↓            ↓ 
SearchPanel[T]     SpecialSearchPanelView
          ↓          ↓
        SearchPanelView

However, I see no reason why it wouldn't be possible to have FieldTemplateSearchPanel inherit only from the FieldTemplateSearchPanelView and FieldTemplateSearchPanelView inherit only from SearchPanel[T], as in the first example. Or, to alternatively have one view, used by multiple controllers implementing different app logic. Or, to maybe not even have a FieldTemplateSearchPanelView and just have a structure like this:

SpecialSearchPanelA     SpecialSearchPanelB
                 ↓       ↓
                SearchPanel
                     ↓
              SearchPanelView

Or am I perhaps missing some constraint here that would prevent these approaches?

[TrigamDev]

Ok I am a bit confused about why the inheritance was done the way it was in this PR...

The way I structured the inheritance was mostly a result of it feeling more natural for a view to inherit from a view and for a controller to inherit from a controller. I'm personally not super familiar with Python, so I wasn't aware about multiple inheritance potentially causing issues, especially since during testing, everything has behaved as expected.

... especially concerning the conclusion that our guidelines cause this, since they are not being followed here.

Aside from an example showing that the controller should inherit from the widget, the guidelines make no mention of anything regarding inheritance.

With this in mind I would have expected a structure like this, where the specialised search panel makes use of a generic search panel with each being split into view and controller:

SpecialSearchPanel
        ↓
SpecialSearchPanelView
        ↓
SearchPanel[T]
        ↓
SearchPanelView

I can refactor to use this inheritance structure instead.

[CyanVoxel]

Important

Above all else I'd like to at least come to an interim consensus on how to unblock this PR, even if it means putting off further inheritance vs composition discussion for later. The future of our MVC recommendations does not have to be decided now, but something needs to be working for this PR in the meantime that (preferably) isn't unknowingly relying on Python's strange approach to multiple inheritance in order to hide underlying issues with Qt widget instantiation.

---

I agree that multiple inheritance should not be used, required, or recommended in order to implement this. I've gone and marked up your inheritance diagrams with the classes used to help further illustrate the polluted inheritance tree that results from this:

Instead this structure was used:

           SpecialSearchPanel
 (Controller, View, Controller, View) # Multiple class copies
             ↓            ↓
    SearchPanel[T]     SpecialSearchPanelView
 (Controller, View)      |   (Controller, View)  # Controller and view are combined prematurely
              ↓          ↓
            SearchPanelView
                 (View) 

To try and clear up some confusion on our current recommendations regarding inheritance, it is my impression that the recommendations say that a controller class should always extend from a view class, with no exceptions regarding base classes designed for code reuse:

# my_cool_widget_controller.py
class MyCoolWidget(MyCoolWidgetView):
    def __init__(self):
        super().__init__()

The above is the only piece of text that mentions inheritance as our method for connecting a view with a controller, and much if not all of our existing MVC code only goes a single layer deep and does not account for reusable base classes.

I'm not sure if you're making exceptions for this in your recommend inheritance structure example, and can't really see how that would apply to the inheritance structure given, but that may shed some light on some of the differences we're seeing with the inheritance approach. But since our current MVC recommendations do not allude to that and I can't see how SearchPanel[T] doesn't have controller logic or how SpecialSearchPanelView doesn't become a controller + view combo that SpecialSearchPanel uses as its view, I have to proceed without that assumption:

With this in mind I would have expected a structure like this, where the specialised search panel makes use of a generic search panel with each being split into view and controller:

SpecialSearchPanel
(Controller+, View+)
        ↓
SpecialSearchPanelView # View has controller logic prematurely
(Controller, View+)
        ↓
SearchPanel[T]
(Controller, View) # Controller and view are combined prematurely
        ↓
SearchPanelView 
(View)

I have issues with this, namely the fact that SpecialSearchPanelView now has pre-implemented controller logic and mixes together the view and controller in a way that directly violates our goal of separating view and controller logic. Furthermore, if we had a case where SearchPanel[T] was not just a generic base class but was a widget that was valid to use on its own, another controller could extend from it without getting view logic polluted in its inheritance chain.

---

With that being said, the code should at least function (as far as I can foresee), and if there's no consensus to use composition right now then I would still accept pulling this PR if it used a functioning inheritance pattern. I just disagree heavily with the approach of controllers becoming views via inheritance after seeing this PR as it seemingly goes against the principles of separating view logic from controller logic (when going more than one class layer deep) and makes code reuse otherwise difficult when controllers have to come along with views before their controller logic is extended by another controller class.

One last appeal for composition over inheritance (on this PR...)

Along with solving all of my above issues with using inheritance as a means for combining controllers and views into widgets, a composition approach would meet all of the goals for MVC you've mentioned, all while not introducing new complexity:

The intention behind our guidelines is that:

• any new widget will be split into controller and view (in order to separate app logic and UI)

Both inheritance and composition provide this

• while still having a single interface, such that other code that is using the widget can't tell that the separation exists.

Both inheritance and composition provide this

• This also means that other code should only ever reference the controller (which is why it should be called MyWidget not MyWidgetController)

Neither strictly prevent this, but at least nudge in that direction

• unless a view is reused in another widget, in which case there should be two controllers inheriting from one view (because the UI logic is the same, while the app logic is different).

Both inheritance and composition provide this

Example of a composition MVC approach

SpecialSearchPanelView
*(View+)*
        ↓
SearchPanelView 
*(View)*

        +

SpecialSearchPanel
*(Controller+)*
        ↓
SearchPanel[T]
*(Controller)*

        =

# View is combined with Controller on instantiation, not hardcoded in the parent class
panel = SpecialSearchPanel(view=SpecialSearchPanelView)

A composition-based revision of our guidelines could then look like this

# my_cool_widget_view.py
class MyCoolWidgetView(QWidget):
    def __init__(self):
        super().__init__()
        self.__button = QPushButton()
        self.__color_dropdown = QComboBox()
        # ...

# my_cool_widget_controller.py
class MyCoolWidget():
    def __init__(self):
        super().__init__(view:MyCoolWidgetView)
        self.view = view # Only if a constant reference is required
        self.__connect_callbacks()

    def __connect_callbacks(self):
        self.view.__button.clicked.connect(self._button_click_callback)
        self.view.__color_dropdown.currentIndexChanged.connect(
            lambda idx: self._color_dropdown_callback(self.__color_dropdown.itemData(idx))
        )

    def _button_click_callback(self):
        print("Button was clicked!")

    def _color_dropdown_callback(self, color: Color):
        print(f"The selected color is now: {color}")

OR

# my_cool_widget_controller.py
class MyCoolWidget():
    def __init__(self):
        super().__init__(view:MyCoolWidgetView)
        # If no constant reference is required, just use the view to connect callbacks
        self.__connect_callbacks(view)

    def __connect_callbacks(self, view):
        self.view.__button.clicked.connect(self._button_click_callback)
        self.view.__color_dropdown.currentIndexChanged.connect(
            lambda idx: self._color_dropdown_callback(self.__color_dropdown.itemData(idx))
        )

    def _button_click_callback(self):
        print("Button was clicked!")

    def _color_dropdown_callback(self, color: Color):
        print(f"The selected color is now: {color}")

This approach has the benefit of letting controllers and views be extended from individually:

• Without mixing controllers and views together prematurely
• Without hard-coding widget implementations into the controller classes
• While allowing views and controllers to be reused and combined however we see fit upon instantiation
• And while adhering to all of our principles for MVC, keeping controller logic and views separated the entire way

[Computerdores]

... especially concerning the conclusion that our guidelines cause this, since they are not being followed here.

Aside from an example showing that the controller should inherit from the widget, the guidelines make no mention of anything regarding inheritance.

Yeah that is my bad, I didn't mean to come across as combative. I had originally had a sentence in there emphasizing that this may not be clear from the docs and that I was specifically referring to the intention behind the MVC guideline, but I must have dropped it accidentally. And yeah, no matter what the consensus on the MVC is in the end, the docs definitely need an improvement there.

With this in mind I would have expected a structure like this, where the specialised search panel makes use of a generic search panel with each being split into view and controller:

I can refactor to use this inheritance structure instead.

This sounds like a good idea to me

[Computerdores]

Above all else I'd like to at least come to an interim consensus on how to unblock this PR, even if it means putting off further inheritance vs composition discussion for later. The future of our MVC recommendations does not have to be decided now, but something needs to be working for this PR in the meantime that (preferably) isn't unknowingly relying on Python's strange approach to multiple inheritance in order to hide underlying issues with Qt widget instantiation.

With that being said, the code should at least function (as far as I can foresee), and if there's no consensus to use composition right now then I would still accept pulling this PR if it used a functioning inheritance pattern. I just disagree heavily with the approach of controllers becoming views via inheritance after seeing this PR as it seemingly goes against the principles of separating view logic from controller logic (when going more than one class layer deep) and makes code reuse otherwise difficult when controllers have to come along with views before their controller logic is extended by another controller class.

I agree, our current lack of consensus on how the MVC should be done in general, shouldn't block this specific PR.
And consequently, as long as the PR doesn't use multiple inheritance in the end, I agree that it will be good to merge.

And anyway, any further changes can only be done once an MVC implementation consensus exists and will by far not be the only UI component that will then need changes, so it wouldn't make sense (or be fair to @TrigamDev) to keep this blocked until then.