openscopeproject / TrguiNG

Remote GUI for Transmission torrent daemon
GNU Affero General Public License v3.0
372 stars 47 forks source link

Colorful + animated progress bars #215

Closed melyux closed 2 months ago

melyux commented 3 months ago

Re-implementation of the PR #82 from last year, on top of the recent change 05303b6. Adds the third possibility of using colorful + animated together (color based on status, animated based on activity):

image

Takes into account all of @qu1ck's suggestions from the original PR. Fits things to match the existing color scheme and the Transmission app:

image

Only applies if user has both "colorful" and "animated" selected for progress bars in interface settings. Other combinations are unaffected by this.

Pentaphon commented 3 months ago

I like this! Hope to see it in the next release.

qu1ck commented 3 months ago

I have yet to test this fully but I noticed one thing right away: you can't just remove a config option, you have to add logic to migrate it from old style enum to your checkboxes to not lose user preferences. There are some examples of this in config class, see openTabs.

And default should be the same: animated, not colorful.

melyux commented 3 months ago

@qu1ck For the deprecated setting in the Settings interface, I'm adding it back as a string since it's stored as a string in the settings JSON. This way, we can migrate the old setting but avoid having to keep its deprecated enum. Good?

        progressbarStyle?: string, // deprecated
        animatedProgressbars: boolean,
        colorfulProgressbars: boolean,
qu1ck commented 3 months ago

Yes, that's fine.

qu1ck commented 3 months ago

Friendly ping. I'd like to cut a release soon and include your PRs in it.

melyux commented 2 months ago

Sure, was on vacation and back now. Halfway done with suggestions

melyux commented 2 months ago

@qu1ck Check out latest changes

melyux commented 2 months ago

Done @qu1ck

qu1ck commented 2 months ago

Looks good, just one question. Was it intentional that when animations are disabled then torrents will be only blue/red/green? Why disable other colors?

melyux commented 2 months ago

I was conservative and just kept the original state of things for the original options, while adding a new combination option. Could expand the new colors to the "colorful" option if we're feeling frisky

qu1ck commented 2 months ago

Well in previous implementation things were either colorful or animated, they couldn't be both. But since this change has 2 different settings for both it makes sense that the difference between [colors on, animations off] and [colors on, animations on] would be only animations, the colors would stay the same.

qu1ck commented 2 months ago

@melyux can you at least comment if you are going to do change discussed above? If you don't have time I can merge this as is and amend later.

melyux commented 2 months ago

Since you approved it as is already I thought it was for a future release. I can make the changes now then

qu1ck commented 2 months ago

Please do. Sorry for the miscommunication and being ambiguous with the approval.

melyux commented 2 months ago

@qu1ck Done with the changes, but since you force pushed the branch (somehow!), it won't let me push again. How can I fix it?

qu1ck commented 2 months ago

I just rebased your branch without changes. You can force push again.

melyux commented 2 months ago

@qu1ck Done

qu1ck commented 2 months ago

Thanks for your contribution and patience. You still have one more PR open :)