-
-
Notifications
You must be signed in to change notification settings - Fork 431
Extend preview panel multi-selection with shared tag editing and update tests #1238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ class TagBoxWidget(TagBoxWidgetView): | |
| on_update = Signal() | ||
|
|
||
| __entries: list[int] = [] | ||
| __mixed_only: bool = False | ||
|
|
||
| def __init__(self, title: str, driver: "QtDriver"): | ||
| super().__init__(title, driver) | ||
|
|
@@ -33,6 +34,38 @@ def __init__(self, title: str, driver: "QtDriver"): | |
| def set_entries(self, entries: list[int]) -> None: | ||
| self.__entries = entries | ||
|
|
||
| def set_mixed_only(self, value: bool) -> None: | ||
| """If True, all tags in this widget are treated as non-shared (grayed out).""" | ||
| self.__mixed_only = value | ||
|
|
||
| def set_tags(self, tags): # type: ignore[override] | ||
| """Render tags; optionally gray out those that are not shared across entries.""" | ||
| tags_ = list(tags) | ||
|
|
||
| # When mixed_only is set, all tags in this widget are considered non-shared. | ||
| shared_tag_ids: set[int] = set() | ||
| if not self.__mixed_only and self.__entries: | ||
| tag_ids = [t.id for t in tags_] | ||
| tag_entries = self.__driver.lib.get_tag_entries(tag_ids, self.__entries) | ||
| required = set(self.__entries) | ||
| for tag_id, entries in tag_entries.items(): | ||
| if set(entries) >= required: | ||
| shared_tag_ids.add(tag_id) | ||
|
|
||
| super().set_tags(tags_) | ||
|
|
||
| # 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) | ||
| 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) | ||
|
Comment on lines
+61
to
+67
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Footnotes
|
||
|
|
||
| @override | ||
| def _on_click(self, tag: Tag) -> None: # type: ignore[misc] | ||
| match self.__driver.settings.tag_click_action: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,8 +111,87 @@ def update_from_entry(self, entry_id: int, update_badges: bool = True): | |
| 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): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 tags and fields from multiple Entry sources, showing shared tags.""" | ||
| logger.warning("[FieldContainers] Updating Multiple Selection", entry_ids=entry_ids) | ||
|
|
||
| entries = list(self.lib.get_entries_full(entry_ids)) | ||
| if not entries: | ||
| self.cached_entries = [] | ||
| self.hide_containers() | ||
| return | ||
|
|
||
| self.cached_entries = entries | ||
|
|
||
| shared_tags = self._get_shared_tags(entries) | ||
|
|
||
| # Compute shared and mixed fields by type id and value. | ||
| all_fields_by_type: dict[int, list[BaseField]] = {} | ||
| for entry in entries: | ||
| for field in entry.fields: | ||
| all_fields_by_type.setdefault(field.type.id, []).append(field) | ||
|
|
||
| shared_fields: list[BaseField] = [] | ||
| mixed_fields: list[BaseField] = [] | ||
| for fields in all_fields_by_type.values(): | ||
| if len(fields) == len(entries) and all(f.value == fields[0].value for f in fields): | ||
| shared_fields.append(fields[0]) | ||
| else: | ||
| mixed_fields.append(fields[0]) | ||
|
|
||
| all_fields: list[BaseField] = shared_fields + mixed_fields | ||
| mixed_field_type_ids: set[int] = {f.type.id for f in mixed_fields} | ||
|
|
||
| self.update_granular( | ||
| shared_tags, | ||
| all_fields, | ||
| update_badges, | ||
| mixed_field_type_ids=mixed_field_type_ids if mixed_field_type_ids else None, | ||
| ) | ||
|
|
||
| # Add a separate container for tags that aren't shared across all entries. | ||
| all_tags: set[Tag] = set() | ||
| for entry in entries: | ||
| all_tags.update(entry.tags) | ||
| mixed_tags: set[Tag] = all_tags - shared_tags | ||
| if mixed_tags: | ||
| index = len(self.containers) | ||
| self.write_tag_container(index, tags=mixed_tags, category_tag=None, is_mixed=True) | ||
|
|
||
| def _get_shared_tags(self, entries: list[Entry]) -> set[Tag]: | ||
| """Get tags that are present in all entries.""" | ||
| if not entries: | ||
| return set() | ||
|
|
||
| shared_tags = set(entries[0].tags) | ||
| for entry in entries[1:]: | ||
| shared_tags &= set(entry.tags) | ||
|
|
||
| return shared_tags | ||
|
|
||
| def _get_shared_fields(self, entries: list[Entry]) -> list[BaseField]: | ||
| """Get fields that are present in all entries with the same value.""" | ||
| if not entries: | ||
| return [] | ||
|
|
||
| shared_fields = [] | ||
| first_entry_fields = entries[0].fields | ||
|
|
||
| for field in first_entry_fields: | ||
| if all( | ||
| any(f.type.id == field.type.id and f.value == field.value for f in entry.fields) | ||
| for entry in entries[1:] | ||
| ): | ||
| shared_fields.append(field) | ||
|
|
||
| return shared_fields | ||
|
|
||
| def update_granular( | ||
| self, entry_tags: set[Tag], entry_fields: list[BaseField], update_badges: bool = True | ||
| self, | ||
| entry_tags: set[Tag], | ||
| entry_fields: list[BaseField], | ||
| update_badges: bool = True, | ||
| mixed_field_type_ids: set[int] | None = None, | ||
| ): | ||
| """Individually update elements of the item preview.""" | ||
| container_len: int = len(entry_fields) | ||
|
|
@@ -131,7 +210,8 @@ def update_granular( | |
|
|
||
| # Write field container(s) | ||
| for index, field in enumerate(entry_fields, start=container_index): | ||
| self.write_container(index, field, is_mixed=False) | ||
| is_mixed = mixed_field_type_ids is not None and field.type.id in mixed_field_type_ids | ||
| self.write_container(index, field, is_mixed=is_mixed) | ||
|
|
||
| # Hide leftover container(s) | ||
| if len(self.containers) > container_len: | ||
|
|
@@ -422,29 +502,36 @@ def write_tag_container( | |
| container.set_title("Tags" if not category_tag else category_tag.name) | ||
| container.set_inline(False) | ||
|
|
||
| if not is_mixed: | ||
| inner_widget = container.get_inner_widget() | ||
| inner_widget = container.get_inner_widget() | ||
|
|
||
| if isinstance(inner_widget, TagBoxWidget): | ||
| with catch_warnings(record=True): | ||
| inner_widget.on_update.disconnect() | ||
| else: | ||
| inner_widget = TagBoxWidget( | ||
| "Tags", | ||
| self.driver, | ||
| ) | ||
| container.set_inner_widget(inner_widget) | ||
|
|
||
| # For mixed tag containers, mark the widget so it can gray out all tags. | ||
| if is_mixed: | ||
| inner_widget.set_mixed_only(True) | ||
| container.set_title(Translations["preview.partial_tags"]) | ||
| else: | ||
| inner_widget.set_mixed_only(False) | ||
|
Comment on lines
+518
to
+522
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| if isinstance(inner_widget, TagBoxWidget): | ||
| with catch_warnings(record=True): | ||
| inner_widget.on_update.disconnect() | ||
| inner_widget.set_entries([e.id for e in self.cached_entries]) | ||
| inner_widget.set_tags(tags) | ||
|
|
||
| def update_callback(): | ||
| if len(self.cached_entries) == 1: | ||
| self.update_from_entry(self.cached_entries[0].id, update_badges=True) | ||
| else: | ||
| inner_widget = TagBoxWidget( | ||
| "Tags", | ||
| self.driver, | ||
| ) | ||
| container.set_inner_widget(inner_widget) | ||
| inner_widget.set_entries([e.id for e in self.cached_entries]) | ||
| inner_widget.set_tags(tags) | ||
| entry_ids = [e.id for e in self.cached_entries] | ||
| self.update_from_entries(entry_ids, update_badges=True) | ||
|
|
||
| inner_widget.on_update.connect( | ||
| lambda: (self.update_from_entry(self.cached_entries[0].id, update_badges=True)) | ||
| ) | ||
| else: | ||
| text = "<i>Mixed Data</i>" | ||
| inner_widget = TextWidget("Mixed Tags", text) | ||
| container.set_inner_widget(inner_widget) | ||
| inner_widget.on_update.connect(update_callback) | ||
|
|
||
| container.set_edit_callback() | ||
| container.set_remove_callback() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)