PICARD-319: Add an action toolbar to file browser #543
Conversation
|
I want to add a couple more buttons to this toolbar
|
|
It would be nice to provide a screenshot for such UI changes in the PR description. |
| else: | ||
| directories.append(path) | ||
|
|
||
| if len(files): |
|
|
||
| if len(files): | ||
| self.tagger.add_files(files) | ||
| if len(directories): |
There was a problem hiding this comment.
if directories: should be enough
|
Screenshot of the same. I plan to add 3 more buttons to that toolbar |
Imho this should be discussed, i'm not an UI guy at all, but i have the feeling adding more buttons don't make things easier. |
|
I modified the code to show text labels under icons, but I don't think that is meant for the filebrowser toolbar.
This is how it looks and I think it kind of defeats the purpose of having a small toolbar. Also the '+' sign is intuitive enough and I have added tooltips for more info. |
|
The + sign is intuitive for the single add-items-to-picard function, but if more buttons are going to be added, not all of them are going to have an obvious meaning. We should take this into consideration because either more buttons are going to be added or the Add one is going to stay the only one. |
|
I am thinking of "Add new folder" button. Which I think will be intuitive as it is used a lot. and a move files here icon like this? |
| if os.path.isfile(path): | ||
| files.append(path) | ||
| else: | ||
| directories.append(path) |
There was a problem hiding this comment.
Why not add directories directly here ?
I understand why you are building a list of files to pass to tagger.add_files(), but you may skip this for directories and just do self.tagger.add_directory(path) here.
| paths = [] | ||
| for index in self.file_browser.selectedIndexes(): | ||
| paths.append(self.file_browser.model.filePath(index)) | ||
| return paths |
There was a problem hiding this comment.
A list comprehension is prolly faster:
return [self.file_browser.model.filePath(index) for index in self.file_browser.selectedIndexes()]
|
Updated look for add items button. I think we can add other buttons after more discussion. |
|
This PR is complete from my side pending any reviews :) |
|
To be honest, I still can't get myself to like that toolbar very much :( On your screenshot it looks really good, but on a higher resolution the toolbar is 70%+ empty whitespace. Maybe this is just because there's only one button now there at the moment... Since I don't have any other suggestions, I'll leave it to zas to decide about this. |
I have the same feeling. I think we can do better, not sure yet how. |
|
Let's postpone this until we have more operations that we'd like to put into that toolbar. |
No description provided.