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?