Extend preview panel multi-selection with shared tag editing and update tests #1238
Conversation
|
When you have an entry selected, select a second, large entry that takes a while to load, and then selecting a third entry before the second loads, the tags and fields will be stuck in a state where they don't update, even when selecting other entries. They will be the tags and fields of the very first entry. |
|
Also, I think it would be nice if tags and fields that aren't shared between all selected entries are still displayed, but grayed out or low opacity |
Thank you for catching that. I believe I've addressed the race condition via timestamp-based cutoff. Regarding the display of non-shared tags, I added a separate "mixed" tag section, which still allows for bulk tag removal. I think this approach is cleaner than graying tags out, but I'm happy to adjust if you prefer a different way forward. |
I should clarify that I'm not representative of Cyan or the project or anything, I'm just speaking as to what I'd personally prefer.
Sadly I'm still experiencing the issue.
Instead of having them display as a category, it might be nicer to instead add a second field box for all the mixed fields. It'd allow the mixed tags to display with categories as well as allow mixed fields to be displayed. It would also be better to tweak the "Showing tags shared by all entries" message to instead say "tags and fields" or something similar. |
Computerdores
left a comment
Computerdores
left a comment
There was a problem hiding this comment.
Generally this is looking good code wise (haven't had time to test yet), but there is a couple of things that could be improved.
(I tried to be verbose in my explanations, but if anything is unclear let me know)
| if is_mixed: | ||
| inner_widget.set_mixed_only(True) | ||
| container.set_title(Translations["preview.partial_tags"]) | ||
| else: | ||
| inner_widget.set_mixed_only(False) |
There was a problem hiding this comment.
this seems like it would cause the wrong title to be shown if inner_widget was previously a non-mixed one, since it doesn't set the title in that case
| self.cached_entries = [entry] | ||
| self.update_granular(entry.tags, entry.fields, update_badges) | ||
|
|
||
| def update_from_entries(self, entry_ids: list[int], update_badges: bool = True): |
There was a problem hiding this comment.
In many other cases in the codebase functionality that can be done for either one or several items is implemented in a single method that checks whether a list or a single item was provided (or if the list only has one item). Doing the same for update_from_entries and update_from_entry would simplify the call in several places
| # Gray out tags that are not shared across all selected entries. | ||
| from tagstudio.qt.mixed.tag_widget import TagWidget # local import to avoid cycles | ||
|
|
||
| layout = getattr(self, "_TagBoxWidgetView__root_layout", None) |
There was a problem hiding this comment.
this is fragile in regards to refactoring and generally regarded as bad practice since it violates separation of concerns.
(When a private variable really needs to accessed from outside of the declaring class it should be marked with a single underscode in the beginning instead, altough it should be carefully considered whether it makes sense in each specific case since it can lead to spaghetti code where every thing is interdependent and it this becomes hard to make changes without unforeseen consequences)
| if layout is not None: | ||
| for i in range(layout.count()): | ||
| item = layout.itemAt(i) | ||
| widget = item.widget() | ||
| if isinstance(widget, TagWidget) and widget.tag: | ||
| if self.__mixed_only or widget.tag.id not in shared_tag_ids: | ||
| widget.setEnabled(False) |
There was a problem hiding this comment.
this violates the core idea of MVC1. UI element related stuff should be handled on the view side not on the controller side. I would suggest moving this to TagBoxWidgetView.set_tags since that is called right before anyway (would also fix the issue from my previous comment, about the access to the private field)
Footnotes
-
See the Style Guide for an explanation ↩
3 participants
Summary
Tasks Completed