fix: subtags/parent tags & aliases update the UI for building a tag #534

DandyDev01 · 2024-10-07T02:30:59Z

The following has moved to #596
subtags can be added and removed when creating or editing a Tag
- parent tags would not be listed before
~~- parent tags could not be removed before~~

can exclude tags from the tag search panel
- it was possible to make a tag parent itself before. Now when selecting a parent tag, the tag being created or edited is excluded from the tag search panel, so it cannot be added as a parent to itself.
~~- before when a tag could parent itself, that tag would be corrupted and users would be unable to edit the tag or add it ~~as a parent to another tag (this would corrupt that tag too)~~~~

- aliases can be removed and added
- aliases can be edited
~~- added support for display name on Tags. This is a parity feature from the json DB.~~

This PR is meant to update the UI for aliases to use a table like the exclude file extensions.

changed how aliases are managed (UI is a table):

yedpodtrzitko · 2024-10-10T15:00:34Z

tagstudio/tests/qt/test_tag_widget.py

-    actions = tag_widget.bg_button.actions()
-    edit_action = [a for a in actions if a.text() == "Edit"][0]
-    edit_action.triggered.emit()
+    # actions = tag_widget.bg_button.actions()
+    # edit_action = [a for a in actions if a.text() == "Edit"][0]
+    # edit_action.triggered.emit()

    # Then
-    panel = tag_box_widget.edit_modal.widget
-    assert isinstance(panel, BuildTagPanel)
-    assert panel.tag.name == tag.name
-    assert panel.name_field.text() == tag.name
+    # NOTE: currently tag_box_widget.edit_modal is never set because its
+    #       edit_tag method is never called
+    # panel = tag_box_widget.edit_modal.widget
+    # assert isinstance(panel, BuildTagPanel)
+    # assert panel.tag.name == tag.name
+    # assert panel.name_field.text() == tag.name


commenting out existing tests isnt really convincing that everything works as expected

Sorry, completely forgot I had done that. Thanks for catching it, the test is properly implemented now.

yedpodtrzitko

please add tests for newly added methods, thanks.

yedpodtrzitko · 2024-10-10T15:01:11Z

.vscode/settings.json

+{
+  "cSpell.words": [
+    "Abendshien",
+    "qtbot",
+    "Voxel"
+  ]
+}


I'd say we want to have whole .vscode in .gitignore

.vscode is now ignored

yedpodtrzitko · 2024-10-12T01:38:39Z

@DandyDev01 the PR is still marked as Draft, so am I supposed to review it already?

DandyDev01 · 2024-10-12T02:14:40Z

@DandyDev01 the PR is still marked as Draft, so am I supposed to review it already?

@yedpodtrzitko Yes, its ready. Sorry I thought I took it off draft when I sent the review request.

seakrueger · 2024-10-12T03:03:26Z

If adding a tag as a parent of itself corrupts the database entry, IMO a check should be added to make sure that isn't possible rather than hiding the tag in the UI.

yedpodtrzitko · 2024-10-12T04:38:44Z

looks good overall, I just noticed some issue with the alias:

open dialog for adding tag
add new input for Alias
click [-] to remove alias (see screenshot below)
it will produce the following error:

Traceback (most recent call last):
  File "/Users/yed/dev/python/TagStudio/tagstudio/src/qt/modals/build_tag.py", line 202, in <lambda>
    new_field = TagAliasWidget(on_remove_callback=lambda a="": self.remove_alias_callback(a))
                                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/yed/dev/python/TagStudio/tagstudio/src/qt/modals/build_tag.py", line 220, in remove_alias_callback
    self.alias_ids.remove(alias_id)
KeyError: None

another issue with the same button when editing existing tag - clicking the [-] on alias seemingly doesnt do anything (ie. the input doesnt disappear), but when saving it, it will actually remove it as expected

yedpodtrzitko

Also could you please squash the commits? thanks.

yedpodtrzitko · 2024-10-13T06:56:56Z

@CyanVoxel can you approve the CI run here?

CyanVoxel · 2024-11-07T20:32:27Z

Thank you for this PR! I apologize for not getting around to commenting on it earlier.

From my understanding, this PR is doing three things:

Connecting the add/edit tag UI for tag aliases and subtags to the SQL backend
Reworking the tag alias UI
Fixing an undocumented bug where a tag could be assigned as its own parent

However I also understand that this PR is not:

Implementing the functionality of tag aliases and subtags (i.e. integrating them into the search system as with 9.4 and below)

The DB connection is still an appreciated addition, and is one step closer to reaching tag feature parity between 9.4 and 9.5. I'd just like to confirm that this was your intention rather than a full re-implementation of these features. If you did intend to re-implement these features so that they're functioning on par with 9.4, then please let me know.

As for the alias UI changes, I have some feedback on the direction taken here. While the previous newline separated textbox wasn't completely ideal, I feel that this individually separated line approach makes things a bit more cumbersome to use. My thoughts are:

New aliases can no longer be quickly created via an Enter press
More vertical screen space is now being taken up by the separated lines
The (-) button overlaps with the beginning of the text, making the text difficult to read and interact with
The (-) button is not aligned with the textbox and does not match the rest of the app theme

My recommendations to solve these issues while keeping the spirit of the changes that improve upon the original implementation would be:

Use some form of table for the alias list, similar to the Include/Exclude filetypes table
Allow the use of the Enter key to automatically create new aliases
Keep a persistent (-) button to the left of each alias, with the default Qt button styling
Allow a backspace on an entry alias text box to remove the alias

DandyDev01 · 2024-11-08T23:54:18Z

@CyanVoxel
Yes that is correct, I did not have intentions to integrate this into the search system as I don't know any of the plans for the search system. edit: I ended up adding search integration for aliases and subtags

In terms of your feedback, the following is my proposed implementation. Please let me know if you approve of them.

New aliases can no longer be quickly created via an Enter press
- ✔create a shortcut CTRL+a to add new aliases for when no other alias has focus. When an alias text box has focus, pressing enter will add a new text box and give it focus
More vertical screen space is now being taken up by the separated lines
- ✔rather than use the include/exclude table, I suggest having the aliases behave like the tags field on entries. I would also make this change to the parent tags. This would avoid having multiple interaction paradigms for tags, helping to keep things simple.

The (-) button overlaps with the beginning of the text, making the text difficult to read and interact with
- ✔your recommendation "Keep a persistent (-) button to the left of each alias, with the default Qt button styling" or on hover will add a button to the left that does not overlap anything.
Allow a backspace on an entry alias text box to remove the alias
-✔ create a shortcut CTRL + d to remove the alias that has focus. Having just backspace could lead to user frustration when editing an alias as they might just delete the alias.

added shortcuts for adding and removing aliases and updated the alias ui to always show remove button and not cover alias aliases use flowlayout wrote test for buildTagPanel removeSelectedAlias parent tags now use flowlayout in build tag panel moved buttons for adding aliases and parents to be at the end of the flowLayout

yedpodtrzitko · 2024-11-13T04:32:31Z

how do I make a query using the tag alias?

…ed or an alias was removed

CyanVoxel

The new alias UI seems to work great! I have no additional comments or requests for that 👍

I noticed that you tried getting the display names working though, which has a couple of issues:

The SQL database structure does not support a consistent parent tag order. Unfortunately switching from a set to a list will not work as a substitute for what will ultimately be needed in the end: which is a an additional db column referencing which parent tag ID should be use to disambiguate it as the display name.
These display names don't seem to function regardless, causing an exception on my machine when trying to access the tag manager panel:

Traceback (most recent call last):
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/tagstudio/src/qt/ts_qt.py", line 368, in <lambda>
    tag_database_action.triggered.connect(lambda: self.show_tag_database())
                                                  ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/tagstudio/src/qt/ts_qt.py", line 666, in show_tag_database
    self.modal.show()
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/tagstudio/src/qt/modals/tag_database.py", line 119, in showEvent
    self.update_tags()
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/tagstudio/src/qt/modals/tag_database.py", line 87, in update_tags
    display_name = tag.display_name
                   ^^^^^^^^^^^^^^^^
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/tagstudio/src/core/library/alchemy/models.py", line 81, in display_name
    if len(self.parent_tags) == 0:
           ^^^^^^^^^^^^^^^^
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/.venv/lib/python3.12/site-packages/sqlalchemy/orm/attributes.py", line 566, in __get__
    return self.impl.get(state, dict_)  # type: ignore[no-any-return]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/.venv/lib/python3.12/site-packages/sqlalchemy/orm/attributes.py", line 1086, in get
    value = self._fire_loader_callables(state, key, passive)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/.venv/lib/python3.12/site-packages/sqlalchemy/orm/attributes.py", line 1121, in _fire_loader_callables
    return self.callable_(state, passive)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/.venv/lib/python3.12/site-packages/sqlalchemy/orm/strategies.py", line 918, in _load_for_state
    raise orm_exc.DetachedInstanceError(
sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <Tag at 0x171e0ea80> is not bound to a Session; lazy load operation of attribute 'parent_tags' cannot proceed (Background on this error at: https://sqlalche.me/e/20/bhk3)

I would suggest just reverting any additions regarding display names here. If you would like to eventually tackle them, I would also suggest taking that on in a separate PR (it's gonna involve some nasty work though so I don't blame you if you'd want to pass on that)

And finally, the bug I mentioned here on #596 is still present. Since this is currently a live bug on main now I would suggest trying to fix that in a new PR that can be merged in the meantime, or at least let me know if you wouldn't like to work on it so that I can open it up to others.

But other than the backend stuff, the UI and UX improvements to the alias list are great!

…d be added in a different PR

DandyDev01 · 2024-12-13T19:55:38Z

I noticed that you tried getting the display names working though, which has a couple of issues:

2. These display names don't seem to function regardless, causing an exception on my machine when trying to access the tag manager panel:
I would suggest just reverting any additions regarding display names here. If you would like to eventually tackle them, I would also suggest taking that on in a separate PR (it's gonna involve some nasty work though so I don't blame you if you'd want to pass on that)

I've just removed them, as you said that is definitely something for a separate PR

And finally, the bug I mentioned here on #596 is still present. Since this is currently a live bug on main now I would suggest trying to fix that in a new PR that can be merged in the meantime, or at least let me know if you wouldn't like to work on it so that I can open it up to others.

I'll create a PR for those changes then

CyanVoxel · 2024-12-22T22:12:36Z

tagstudio/src/core/library/alchemy/models.py

+        primaryjoin="Tag.id == TagSubtag.parent_id",
+        secondaryjoin="Tag.id == TagSubtag.child_id",


Any specific reason for this change?

CyanVoxel · 2024-12-22T22:12:51Z

tagstudio/src/core/library/alchemy/library.py

@@ -618,7 +618,6 @@ def search_tags(
                results=len(res),
            )

-            session.expunge_all()


Any reason for this as well?

CyanVoxel · 2024-12-22T22:15:09Z

tagstudio/src/qt/widgets/thumb_renderer.py

@@ -12,7 +12,6 @@

 import cv2
 import numpy as np
-import pillow_jxl  # noqa: F401


Was removing this import accidental? This is used for JPEG XL support

session.expunge_all() on line 621 was removed, added it back.

parent_tags primaryJoin and secondaryJoin where in the wrong order. They have been switched back to the proper order.

CyanVoxel · 2024-12-23T01:36:13Z

Not sure why the MyPy check is failing here as it passes on my machines, looking into it now.
Maybe this just needs an update from main?

CyanVoxel · 2024-12-23T01:48:09Z

The MyPy workflow is failing on this, though I still don't know why I can't reproduce it locally:

/home/runner/work/TagStudio/TagStudio/tagstudio/src/qt/modals/tag_database.py:87:32: error: Argument "subtag_ids" to "add_tag" of "Library" has incompatible type "list[int]"; expected "set[int] | None"  [arg-type]
/home/runner/work/TagStudio/TagStudio/tagstudio/src/qt/modals/tag_database.py:88:33: error: Argument "alias_names" to "add_tag" of "Library" has incompatible type "list[str]"; expected "set[str] | None"  [arg-type]
/home/runner/work/TagStudio/TagStudio/tagstudio/src/qt/modals/tag_database.py:89:31: error: Argument "alias_ids" to "add_tag" of "Library" has incompatible type "list[int]"; expected "set[int] | None"  [arg-type]
Found 3 errors in 1 file (checked 109 source files)'

CyanVoxel · 2024-12-23T04:49:51Z

I'm suspecting that something is wrong with the workflow and isn't running on this PR properly, as these line numbers only make sense when looking at the current main code and not this PR.

CyanVoxel · 2024-12-23T05:58:14Z

So it's not a workflow issue - MyPy just creates a new merge commit and sees that the type hint changes here are conflicting with a call to add_tag() that uses lists that was merged into main a couple days ago. MyPy can't comment on what's wrong here since this PR isn't up to date with main, and thus doesn't have access to the new code that conflicts with these changes.

CyanVoxel

Thank you for all your work on this! I apologize for the long turnaround time and roundabout PRs for this, but I appreciate you working with me and I love the final results here!

DandyDev01 changed the title ~~SQLite DB with SQLAlchemy II #332 subtags/parent tags~~ SQLite DB with SQLAlchemy subtags/parent tags Oct 7, 2024

DandyDev01 marked this pull request as draft October 7, 2024 02:46

CyanVoxel added this to the SQL Parity milestone Oct 7, 2024

CyanVoxel added TagStudio: Library Relating to the TagStudio library system TagStudio: Tags Relating to the TagStudio tag system labels Oct 7, 2024

DandyDev01 changed the title ~~SQLite DB with SQLAlchemy subtags/parent tags~~ SQLite DB with SQLAlchemy subtags/parent tags & aliases Oct 9, 2024

DandyDev01 marked this pull request as ready for review October 10, 2024 01:09

yedpodtrzitko reviewed Oct 10, 2024

View reviewed changes

yedpodtrzitko suggested changes Oct 10, 2024

View reviewed changes

DandyDev01 marked this pull request as draft October 10, 2024 15:41

DandyDev01 requested a review from yedpodtrzitko October 10, 2024 23:58

DandyDev01 marked this pull request as ready for review October 12, 2024 02:11

yedpodtrzitko suggested changes Oct 12, 2024

View reviewed changes

DandyDev01 closed this Oct 12, 2024

DandyDev01 force-pushed the main branch from 251ee08 to abeb0c1 Compare October 12, 2024 19:12

DandyDev01 reopened this Oct 12, 2024

DandyDev01 requested a review from yedpodtrzitko October 23, 2024 01:47

yedpodtrzitko approved these changes Oct 23, 2024

View reviewed changes

CyanVoxel added the Priority: High An important issue requiring attention label Nov 9, 2024

DandyDev01 requested a review from yedpodtrzitko November 12, 2024 21:58

DandyDev01 force-pushed the main branch from ace6f20 to 1b12482 Compare November 13, 2024 02:53

DandyDev01 added 2 commits November 30, 2024 16:13

fix: alias name changes would be discarded when a new alias was creat…

bb39a3f

…ed or an alias was removed

merge with upstream/main

979c6f3

CyanVoxel mentioned this pull request Dec 5, 2024

Add Tag Modal: Keyboard nav improvements #407

Open

DandyDev01 marked this pull request as ready for review December 6, 2024 22:26

DandyDev01 requested a review from CyanVoxel December 9, 2024 22:22

CyanVoxel requested changes Dec 10, 2024

View reviewed changes

DandyDev01 force-pushed the main branch from 7572f8a to e87403c Compare December 13, 2024 19:48

fix: removed display names, as they didn't display properly and shoul…

38b0e5d

…d be added in a different PR

DandyDev01 force-pushed the main branch from e87403c to 38b0e5d Compare December 13, 2024 19:52

DandyDev01 requested a review from CyanVoxel December 13, 2024 19:55

DandyDev01 force-pushed the main branch 2 times, most recently from c0e01ff to 38b0e5d Compare December 13, 2024 20:21

DandyDev01 added 2 commits December 13, 2024 13:23

fix: mypy

fac0c8d

Merge remote-tracking branch 'upstream/main'

e046681

CyanVoxel reviewed Dec 22, 2024

View reviewed changes

DandyDev01 and others added 4 commits December 22, 2024 16:53

fix: added missing session.expunge_all

0f7ba0c

session.expunge_all() on line 621 was removed, added it back.

fix: parent_tags relationship in Tag class

3b0a423

parent_tags primaryJoin and secondaryJoin where in the wrong order. They have been switched back to the proper order.

fix: pillow_jxl import was missing

e877eba

fix: ruff

e317509

CyanVoxel added this to the Alpha v9.5 (Post-SQL) milestone Dec 23, 2024

CyanVoxel added 2 commits December 22, 2024 22:05

fix: type hint fixes

1be7e49

fix: fix the type hint fixes

715d10e

CyanVoxel removed the Status: Changes Requested Changes are quested to this label Dec 23, 2024

CyanVoxel approved these changes Dec 23, 2024

View reviewed changes

CyanVoxel merged commit 82c659c into TagStudioDev:main Dec 23, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: subtags/parent tags & aliases update the UI for building a tag #534

fix: subtags/parent tags & aliases update the UI for building a tag #534

DandyDev01 commented Oct 7, 2024 •

edited

Loading

yedpodtrzitko Oct 10, 2024

DandyDev01 Oct 10, 2024 •

edited

Loading

yedpodtrzitko left a comment

yedpodtrzitko Oct 10, 2024

DandyDev01 Oct 10, 2024

yedpodtrzitko commented Oct 12, 2024

DandyDev01 commented Oct 12, 2024

seakrueger commented Oct 12, 2024

yedpodtrzitko commented Oct 12, 2024 •

edited

Loading

yedpodtrzitko left a comment

yedpodtrzitko commented Oct 13, 2024

CyanVoxel commented Nov 7, 2024

DandyDev01 commented Nov 8, 2024 •

edited

Loading

yedpodtrzitko commented Nov 13, 2024

CyanVoxel left a comment

DandyDev01 commented Dec 13, 2024

CyanVoxel Dec 22, 2024

CyanVoxel Dec 22, 2024

CyanVoxel Dec 22, 2024

CyanVoxel commented Dec 23, 2024

CyanVoxel commented Dec 23, 2024

CyanVoxel commented Dec 23, 2024 •

edited

Loading

CyanVoxel commented Dec 23, 2024

CyanVoxel left a comment

		primaryjoin="Tag.id == TagSubtag.parent_id",
		secondaryjoin="Tag.id == TagSubtag.child_id",

fix: subtags/parent tags & aliases update the UI for building a tag #534

fix: subtags/parent tags & aliases update the UI for building a tag #534

Conversation

DandyDev01 commented Oct 7, 2024 • edited Loading

yedpodtrzitko Oct 10, 2024

Choose a reason for hiding this comment

DandyDev01 Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

yedpodtrzitko left a comment

Choose a reason for hiding this comment

yedpodtrzitko Oct 10, 2024

Choose a reason for hiding this comment

DandyDev01 Oct 10, 2024

Choose a reason for hiding this comment

yedpodtrzitko commented Oct 12, 2024

DandyDev01 commented Oct 12, 2024

seakrueger commented Oct 12, 2024

yedpodtrzitko commented Oct 12, 2024 • edited Loading

yedpodtrzitko left a comment

Choose a reason for hiding this comment

yedpodtrzitko commented Oct 13, 2024

CyanVoxel commented Nov 7, 2024

DandyDev01 commented Nov 8, 2024 • edited Loading

yedpodtrzitko commented Nov 13, 2024

CyanVoxel left a comment

Choose a reason for hiding this comment

DandyDev01 commented Dec 13, 2024

CyanVoxel Dec 22, 2024

Choose a reason for hiding this comment

CyanVoxel Dec 22, 2024

Choose a reason for hiding this comment

CyanVoxel Dec 22, 2024

Choose a reason for hiding this comment

CyanVoxel commented Dec 23, 2024

CyanVoxel commented Dec 23, 2024

CyanVoxel commented Dec 23, 2024 • edited Loading

CyanVoxel commented Dec 23, 2024

CyanVoxel left a comment

Choose a reason for hiding this comment

DandyDev01 commented Oct 7, 2024 •

edited

Loading

DandyDev01 Oct 10, 2024 •

edited

Loading

yedpodtrzitko commented Oct 12, 2024 •

edited

Loading

DandyDev01 commented Nov 8, 2024 •

edited

Loading

CyanVoxel commented Dec 23, 2024 •

edited

Loading