-
Notifications
You must be signed in to change notification settings - Fork 386
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
feat(qt): Add hotkey to add tags to selection #302
Conversation
This is used so that you don't have to go through the mouse-heavy nitty-gritty of: 1. Select one or more files 2. Add field 3. Tags 4. OK 5. Add Tag 6. Click on the plus sign 7. Done To get to a tag selection modal. This reduces a lot of friction in my opinion. I like the way that the fields can have multiple types, but in the majority of cases I just want to quickly tag one or more files in my selection and move on. The workflow after this change is as follows: 1. Select one or more files 2. Just press `Ctrl+Shift+T` 3. Click on the plus sign 4. Done This way the tagbox gets added automatically (through the powerful core library system) and you can immediately start tagging!
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.
Its a bit nitpicky but i think it should make it more readable.
add_tag_action = QAction("Add Tag To File", menu_bar) | ||
add_tag_action.triggered.connect(lambda: self.attach_tag_action_callback()) | ||
add_tag_action.setShortcut( | ||
QtCore.QKeyCombination( |
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.
I think you could remove the QtCore. by importing QKeyCombination directly, which makes it more readable i think.
add_tag_action.triggered.connect(lambda: self.attach_tag_action_callback()) | ||
add_tag_action.setShortcut( | ||
QtCore.QKeyCombination( | ||
QtCore.Qt.KeyboardModifier( |
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.
Here (and in the following lines) you could remove the QtCore. and just use for example Qt.KeyboardModifier (Qt is already imported in line 28)
@@ -384,6 +385,20 @@ def start(self) -> None: | |||
new_tag_action.setToolTip("Ctrl+T") | |||
edit_menu.addAction(new_tag_action) | |||
|
|||
add_tag_action = QAction("Add Tag To File", menu_bar) | |||
add_tag_action.triggered.connect(lambda: self.attach_tag_action_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.
I think you could remove the lambda and just use add_tag_action.triggered.connect(self.attach_tag_action_callback)
, because you don't provide any input to the method. (I know that this is also present in the actions above yours and i think these changes should also be made there, but i don't know if that would be out of scope for this pr)
I agree with your proposed changes, however, I think that those issues should be fixed in one fell swoop as per a refactor changeset. I don't think that should be included in a feature PR. Do as you want though; maybe I'll look at the refactoring I could do later on. |
To add to that: I've written my PR consciously in this way, as to stay consistent with the existing code. I know that constitutes "copy-paste code", but I thought it was appropriate here. |
Yeah i thougt that and you are probably right that the refactoring part is better of in another pr. |
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.
I agree with the other comments, but it is still fair to use existing code conventions. I approve.
Unfortunately this is going to need some refactoring to work with the new SQL backend (#332). I understand this PR has been open for a while now, and I apologize for taking a while to get around to it. The merge conflict here is pretty minor, and from what I can tell the I love the idea of this feature, and it brings things one step closer to being fully keyboard controlled. My only gripe is that there's no way to choose which type of Tag Field the tag gets added to. Personally I use "Meta Tags" and "Content Tags" in my library, so I would effectively be locked out of using such a great shortcut. I'm also obligated to mention that the entire "Tag Fields" concept will eventually be replaced by automatic tag categories in a future update, so issues like this will no longer be a concern. |
Made obsolete by #655 which removed tag fields in favor of applying tags directly to entries and then automatically sorting them by category |
Oops, haven't looked back at this PR for a while now. Good to hear things are better overall for the software. Apologies accepted 😂 |
This is used so that you don't have to go through the mouse-heavy
nitty-gritty of:
To get to a tag selection modal. This reduces a lot of friction in my
opinion. I like the way that the fields can have multiple types, but in
the majority of cases I just want to quickly tag one or more files in my
selection and move on. The workflow after this change is as follows:
Ctrl+Shift+T
This way the tagbox gets added automatically (through the powerful core
library system) and you can immediately start tagging!