TagStudioDev / TagStudio

A User-Focused Photo & File Management System
GNU General Public License v3.0
2.85k stars 266 forks source link

add list of libraries into sidebar #151

Closed yedpodtrzitko closed 1 month ago

yedpodtrzitko commented 1 month ago

superseding #127

...working with the variable is now moved into the same place.

CyanVoxel commented 1 month ago

I like the ability to toggle the automatic library loading on start. I saw in #127 you said you motivation for this was the thumbnail threads spooling up, which is more of an issue with the current lack of thumbnail caching than anything else. That being said, there will always be some resources being used when opening a library and having the choice to toggle that off is valuable regardless.

While I largely agree with @Loran425 about the approach of having an "Open Recent Libraries" menu over the idea of having a persistent panel on screen, I can see the desire to be able to switch between libraries faster. As long as it's able to be hidden for users who don't want it, I don't see the harm in it. With that being said though, I can't seem to get it to even appear on my end: image

In addition, the splash screen seems to be broken for me now, which could be related to the changes in ts_qt.py or could just be some Qt resource shenanigans. image

yedpodtrzitko commented 1 month ago

While I largely agree with @Loran425 about the approach of having an "Open Recent Libraries" menu over the idea of having a persistent panel on screen, I can see the desire to be able to switch between libraries faster. As long as it's able to be hidden for users who don't want it, I don't see the harm in it.

that's not configurable via the following menu item:

Screenshot 2024-05-11 at 22 56 53

With that being said though, I can't seem to get it to even appear on my end:

That should be fixed now. It got lost when checking in new commits. Thanks for catching that.

In addition, the splash screen seems to be broken for me now, which could be related to the changes in ts_qt.py or could just be some Qt resource shenanigans.

I never see the splash screen (even before), so I cant test it, but I made a speculative change which could fix it.

Let me know if it works for you now as expected, thanks.

CyanVoxel commented 1 month ago

Currently there's an issue on Windows where the path separators aren't normalized list the library list, resulting in duplicate libraries showing up in the list: image Normally I would recommend wrapping the paths in os.path.normpath(), however #156 is trying to remove the use of os.path in favor of the pathlib library.

Also, the splash screen is still broken for me but I'll see if I can make changes to get that functional on at least my end.

CyanVoxel commented 1 month ago

I'll be honest, I was skeptical at first of whether I would personally use this list vs just having it tucked away in the File menu, but as soon as I actually used it I didn't want to live without it 😁 Fantastic work on this so far!

Other than the issues and styling changes commented above, I wanted to suggest a few other changes/additions. These tie into the idea of this being more of a "Recent Libraries" list rather than a list of all libraries you've historically opened.

Proposed Changes/Additions

  1. Automatic reordering of libraries based on the order of last opening. For example if you were to click on a library in the middle of the list to open it, then it would move to the top.
  2. Right-click context menu option to remove a library from the list.
  3. A limit for how many libraries show up in this list. I think a hardcoded limit of 5 would work well, then once we get a proper settings panel then this can be user-configurable.

Thank you again for all your work on this!

yedpodtrzitko commented 1 month ago

Currently there's an issue on Windows where the path separators aren't normalized list the library list, resulting in duplicate libraries showing up in the list:

I'm afraid I dont have any windows machine around here, so I cant reproduce this issue. I just hope the Pathlib PR will solve this automagically.

yedpodtrzitko commented 1 month ago

Right-click context menu option to remove a library from the list.

I was thinking to implement this in future, but rather than using the context menu, I was considering to add there another button, the same way as Tags have it (➖ ), or Fields have it (🗑️ ), so the design language is somehow consistent.

CyanVoxel commented 1 month ago

Right-click context menu option to remove a library from the list.

I was thinking to implement this in future, but rather than using the context menu, I was considering to add there another button, the same way as Tags have it (➖ ), or Fields have it (🗑️ ), so the design language is somehow consistent.

Makes sense to me, I thing the (➖) would be the better route.

CyanVoxel commented 1 month ago

Not sure if you're running into this issue as well, but when using the refactored color variable enums:

scroll_area.setStyleSheet(
            "QWidget#entryScrollContainer{"
            f"background: {Theme.COLOR_BG};"
            "border-radius:6px;"
            "}"
        )

Qt throws an error trying to read from the stylesheets because it's using the literal name "Theme.COLOR_BG" as the color value. If I change these instances to Theme.COLOR_BG.value then it works as intended.

yedpodtrzitko commented 1 month ago

Not sure if you're running into this issue as well, but when using the refactored color variable enums:

scroll_area.setStyleSheet(
            "QWidget#entryScrollContainer{"
            f"background: {Theme.COLOR_BG};"
            "border-radius:6px;"
            "}"
        )

Qt throws an error trying to read from the stylesheets because it's using the literal name "Theme.COLOR_BG" as the color value. If I change these instances to Theme.COLOR_BG.value then it works as intended.

No issue on my side, but adding the .value explicitly wont hurt.

yedpodtrzitko commented 1 month ago

Looks great!! I attempted to find a way to make the library splitter section only take up the minimum amount of space it needs instead of letting it grow along the title label, but couldn't find anything quickly that would do it. Given that it would only be a minor visual enhancement I won't worry about it too much right now. If you or I can't find a solution to it soon then perhaps opening up an issue for it after the fact would be best. After all, I think we'd both rather move onto more important fixes and additions than spend too much time fighting with Qt ;)

seems like that got lost during rebasing and I didnt check after last push. It should be fixed now by button_remove.setFixedWidth(30)

CyanVoxel commented 1 month ago

seems like that got lost during rebasing and I didnt check after last push. It should be fixed now by button_remove.setFixedWidth(30)

The button_remove width issue wasn't actually present until I pulled just now to see what fix you were talking about. I was referring to the excess space that can surround the "Recent Libraries" label instead of having the maximum height of the section be equal to it's minimum height at any given time. image

yedpodtrzitko commented 1 month ago

The button_remove width issue wasn't actually present until I pulled just now to see what fix you were talking about. I was referring to the excess space that can surround the "Recent Libraries" label instead of having the maximum height of the section be equal to it's minimum height at any given time.

ok I see what you mean now. Check the behaviour after the latest commit I pushed.

CyanVoxel commented 1 month ago

ok I see what you mean now. Check the behaviour after the latest commit I pushed.

Awesome!! Works exactly how I'd expect! Every now and again it won't update the size when when I remove a library, but I can't predictably reproduce it. Overall though I'm more than happy to go ahead and pull this. Thank you again for all your hard work!