Add support for 'tags_exclude' to let user specify tags to exclude photos#545
Open
greensum wants to merge 1 commit intohelgeerbe:mainfrom
Open
Add support for 'tags_exclude' to let user specify tags to exclude photos#545greensum wants to merge 1 commit intohelgeerbe:mainfrom
greensum wants to merge 1 commit intohelgeerbe:mainfrom
Conversation
photos from being displayed. Like the 'tags_filter', it can be set in the configuration.yaml file, through MQTT, and through HTTP. This is needed even though the 'tags_filter' can take expressions like 'NOT (private OR hidden)' as that would only select photos that have at lease some tags, as long as they are not 'private' nor 'hidden'. With 'tags_exclude' it will show photos that don't have any tags, and of course it will exclude photo that have the matching tags.
Reviewer's GuideIntroduces a new tags_exclude filter analogous to tags_filter by adding its configuration, integrating it into the data model, controller, MQTT and HTTP interfaces, UI elements, and example configuration to let users exclude photos by tag (including untagged items). Sequence diagram for setting tags_exclude via MQTTsequenceDiagram
participant MQTT
participant "interface_mqtt"
participant Controller
participant Model
MQTT->>interface_mqtt: Send /tags_exclude message
interface_mqtt->>Controller: Set tags_exclude
Controller->>Model: Set tags_exclude
Model->>Model: Update filter and reload files
Sequence diagram for setting tags_exclude via HTTPsequenceDiagram
actor User
participant HTTP_Interface
participant Controller
participant Model
User->>HTTP_Interface: Send request to set tags_exclude
HTTP_Interface->>Controller: Set tags_exclude
Controller->>Model: Set tags_exclude
Model->>Model: Update filter and reload files
Class diagram for updated Model and Controller with tags_exclude supportclassDiagram
class Model {
- location_filter: str
- tags_filter: str
- tags_exclude: str
+ get_viewer_config()
+ tags_filter
+ tags_exclude
}
class Controller {
- __model: Model
+ tags_filter
+ tags_exclude
}
Model <|-- Controller
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/picframe/model.py:325` </location>
<code_context>
+ @tags_exclude.setter
+ def tags_exclude(self, val):
+ self.__config['model']['tags_exclude'] = val
+ if len(val) > 0:
+ ftr = self.__build_filter(val, "tags")
+ if ftr:
</code_context>
<issue_to_address>
Using 'len(val) > 0' assumes val is always a string.
If 'tags_exclude' can be non-string (e.g., None), this may cause an error. Use 'if val:' or ensure 'val' is always a string.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if len(val) > 0:
ftr = self.__build_filter(val, "tags")
if ftr:
ftr = "(tags IS NULL OR NOT {})".format(ftr)
self.set_where_clause("tags_exclude", ftr)
else:
self.set_where_clause("tags_exclude") # remove from where_clause
self.__reload_files = True
=======
if val:
ftr = self.__build_filter(val, "tags")
if ftr:
ftr = "(tags IS NULL OR NOT {})".format(ftr)
self.set_where_clause("tags_exclude", ftr)
else:
self.set_where_clause("tags_exclude") # remove from where_clause
self.__reload_files = True
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/picframe/interface_mqtt.py:719` </location>
<code_context>
self.__logger.info("Recieved tags filter: %s", msg)
self.__controller.tags_filter = msg
+ # tags exclude filter
+ elif message.topic == self.__device_id + "/tags_exclude":
+ self.__logger.info("Recieved tags exclude filter: %s", msg)
+ self.__controller.tags_exclude = msg
</code_context>
<issue_to_address>
Typo in log message: 'Recieved' should be 'Received'.
Fixing the typo ensures clear and professional log output.
Suggested implementation:
```python
self.__logger.info("Received tags filter: %s", msg)
```
```python
self.__logger.info("Received tags exclude filter: %s", msg)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+325
to
+332
| if len(val) > 0: | ||
| ftr = self.__build_filter(val, "tags") | ||
| if ftr: | ||
| ftr = "(tags IS NULL OR NOT {})".format(ftr) | ||
| self.set_where_clause("tags_exclude", ftr) | ||
| else: | ||
| self.set_where_clause("tags_exclude") # remove from where_clause | ||
| self.__reload_files = True |
There was a problem hiding this comment.
suggestion (bug_risk): Using 'len(val) > 0' assumes val is always a string.
If 'tags_exclude' can be non-string (e.g., None), this may cause an error. Use 'if val:' or ensure 'val' is always a string.
Suggested change
| if len(val) > 0: | |
| ftr = self.__build_filter(val, "tags") | |
| if ftr: | |
| ftr = "(tags IS NULL OR NOT {})".format(ftr) | |
| self.set_where_clause("tags_exclude", ftr) | |
| else: | |
| self.set_where_clause("tags_exclude") # remove from where_clause | |
| self.__reload_files = True | |
| if val: | |
| ftr = self.__build_filter(val, "tags") | |
| if ftr: | |
| ftr = "(tags IS NULL OR NOT {})".format(ftr) | |
| self.set_where_clause("tags_exclude", ftr) | |
| else: | |
| self.set_where_clause("tags_exclude") # remove from where_clause | |
| self.__reload_files = True |
Comment on lines
+719
to
+720
| elif message.topic == self.__device_id + "/tags_exclude": | ||
| self.__logger.info("Recieved tags exclude filter: %s", msg) |
There was a problem hiding this comment.
nitpick (typo): Typo in log message: 'Recieved' should be 'Received'.
Fixing the typo ensures clear and professional log output.
Suggested implementation:
self.__logger.info("Received tags filter: %s", msg) self.__logger.info("Received tags exclude filter: %s", msg)| if len(val) > 0: | ||
| ftr = self.__build_filter(val, "tags") | ||
| if ftr: | ||
| ftr = "(tags IS NULL OR NOT {})".format(ftr) |
There was a problem hiding this comment.
suggestion (code-quality): Replace call to format with f-string (use-fstring-for-formatting)
Suggested change
| ftr = "(tags IS NULL OR NOT {})".format(ftr) | |
| ftr = f"(tags IS NULL OR NOT {ftr})" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As a follow up to my discussion on my need to exclude some photos, I made a simple enhancement to add the support for specifying tags to exclude. This is needed even though the existing 'tags_filter' can take expressions like 'NOT (private OR hidden)' as that would only select photos that have at least some tags, as long as they are not 'private' nor 'hidden'. With 'tags_exclude' it will show photos that don't have any tags, and of course it will exclude photo that have the matching tags. Like the 'tags_filter', 'tags_exclude' can be set in the configuration.yaml file, through MQTT, and through HTTP.
Summary by Sourcery
Add a new tags_exclude filter to allow users to exclude photos by tag across all interfaces
New Features:
Enhancements: