Skip to content
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

Places color chooser script #145

Merged
merged 23 commits into from
Jan 10, 2025
Merged

Conversation

akarzim
Copy link
Contributor

@akarzim akarzim commented Jan 3, 2025

See discussion #89 (comment)

Usage

❯ ./scripts/folders-color-chooser -h  
Folders color chooser

Current color: pistachio

Usage:
  folders-color-chooser [-c | --color=<color>] [-C | --reset-color]
  folders-color-chooser [-h | --help]
  folders-color-chooser [-l | --list]

Options:
  -c, --color=<color>   set the new folders color
  -C, --reset-color     reset folder color to plasma
  -h, --help            show this help
  -l, --list            list available colors

Output

❯ ./scripts/folders-color-chooser -C
No changes. It's already plasma.
❯ ./scripts/folders-color-chooser -l      
Available colors are:
black blue citron firebrick gold green grey highland jade lavender lime olive orange pistachio plasma pumpkin purple red rust sapphire tomato violet white yellow
❯ ./scripts/folders-color-chooser -c
Missing value for 'c' option. Please provide a color.
Available colors are:
black blue citron firebrick gold green grey highland jade lavender lime olive orange pistachio plasma pumpkin purple red rust sapphire tomato violet white yellow
❯ ./scripts/folders-color-chooser -c gold
Changed from plasma to gold.
❯ ./scripts/folders-color-chooser anything
Unknown argument: anything

@SylEleuth
Copy link
Owner

Who knows, maybe a high-contrast colour or something will be added in the future; maybe someone will be happy to add their own colour to the set and use this script seamlessly!

In my opinion colour palette should be immutable, if someone wants to add new colours or change them even a little bit then should create a new scheme (Gruvbox has several different ones). Adding new colours at this point would not be easy (and I have no plans to do that). I know what I'm doing and know icons through and out and it was difficult and arduous for me to create all colour sets. That's the whole point of the plasma "colour". It's not only specific for KDE Plasma users, but its construction allows to easily change it to any colour by just modifying .ColorScheme-Text variable to anything You want.

I find it reassuring, as a user, to be able to know what will happen with a destructive script before running it for real.

Still not convinced.

Some programs allow multiple levels of verbosity

That idea is even more confusing. The script should be as easy as it's possible. Many users wouldn't even know that they have to change permission of the script to use it. They don't know the structure of the icons, what folders are there, and definitely what icons should link to what name, and if it's correct (even I'm not sure just looking on the list, had to compare with my own). They are just copy it to specific folder and change it in the DE settings (and folder colours if they are using KDE Plasma). What they need to know is what colour to choose from and if everything went fine. They will see actual colours on the github page and use that name in the script, because they are hard to identify just by the name (every one has several shades, e.g. green, highland, pistachio). That's all they need to know. Lets not make it too complicated for anyone.

I've placed this script in a scripts/ directory at the root of the project, and the icons directory is defined in relation to this script

Script won't be a part of the release and that is how most people install it (and through DE settings). Icon pack will contain only icons with the cache and index.theme. If user want to use it would have to download it or clone it and then copy to the places/scalable. Creating the scripts folder within icons folder is not advisable because it's a place for icons only. If You can make it actually usable from anywhere would be fantastic.

I don't know about Debian because I don't know much about that distro and no one offered to maintain icons there yet, but Arch doesn't allow to install anywhere outside root (/usr/share/icons/).

I've always assumed that default user's location would be .local/share/icons instead of .icons. It would make 3 places where to seek for the pack.

Could You change the name of the script? Use folder instead of places? I'm still thinking about something shorter.

The help info looks ok. It says that plasma is default, but if colour was already changed that would be confusing. Maybe instead of that it would display a current colour?

@akarzim
Copy link
Contributor Author

akarzim commented Jan 4, 2025

In my opinion colour palette should be immutable.

I agree with you. If it suits you better, let's hard-code the colours.

Still not convinced.

Okay. Let's remove the dry-run option.

That idea is even more confusing. The script should be as easy as it's possible.

So let's get rid of all the verbosity.

Arch doesn't allow to install anywhere outside root (/usr/share/icons/).
I've always assumed that default user's location would be .local/share/icons instead of .icons. It would make 3 places where to seek for the pack.

It seems we need to check more places:

  • $XDG_DATA_HOME (defaults to $HOME/.local/share)
  • $HOME/.icons (for backwards compatibility)
  • $XDG_DATA_DIRS/icons (defaults to /usr/local/share/icons:/usr/share/icons)

Cf. https://specifications.freedesktop.org/icon-theme-spec/latest/#directory_layout
Cf. https://specifications.freedesktop.org/basedir-spec/latest/#variables

Could You change the name of the script? Use folder instead of places? I'm still thinking about something shorter.

Of course I will. I find folder-color-chooser self-explanatory, but if you prefer another name, I'll change it. Note that the .sh extension is dispensable.

I can also change the American "color" to the British "colour" throughout the script if you wish. 😇

The help info looks ok. It says that plasma is default, but if colour was already changed that would be confusing. Maybe instead of that it would display a current colour?

It's already there! The line just before the usage section says:

Current color: plasma

This isn't the best example, as it's the default colour, but it is the current colour. 😅

@SylEleuth
Copy link
Owner

I don't want to be a pain in the arse. I just want to discuss it first. I don't mind a debate over features.

I find folder-color-chooser self-explanatory

That is why I asked you to change it from places-color-chooser.

Colour in my vocabulary is just a habit. Color is internet's default. Don't change it.

@akarzim
Copy link
Contributor Author

akarzim commented Jan 4, 2025

I don't want to be a pain in the arse. I just want to discuss it first. I don't mind a debate over features.

You're not, really 🤗 I just want you to be comfortable with the final result. In the meantime, I've been learning things about bash scripting, and that's what really matters to me 😊

Colour in my vocabulary is just a habit. Color is internet's default. Don't change it.

Fine.

I've done everything we talked about. We shouldn't be too far from the final script!

@SylEleuth
Copy link
Owner

SylEleuth commented Jan 7, 2025

It works great.

Icon pack path: is unnecessary I think, same with ICON_PACK_THEME name of the Gruvbox Plus icon pack (default: Gruvbox-Plus-Dark). And You could remove =FOLDERS_COLOR from options. It's confusing and suggesting that You'd have to use = after -c. Make it simple.

When used without any arguments it resets colour to plasma. Maybe it could just display help info?

Can You add empty line every piece of information? It would look something like that:

Folders color chooser

Current color: firebrick

Usage: places-color-chooser.sh [-c | --color] FOLDERS_COLOR [-h | --help] [-l | --list]

Environment:
  FOLDERS_COLOR     color to change to (default: plasma)

Options:
  -c, --color       set the new folders color (default: plasma)
  -h, --help        show this help
  -l, --list        list available colors

@akarzim
Copy link
Contributor Author

akarzim commented Jan 7, 2025

Icon pack path: is unnecessary I think, same with ICON_PACK_THEME name of the Gruvbox Plus icon pack (default: Gruvbox-Plus-Dark).

I removed icon pack theme option. We'll add it again if anyone asks.

And You could remove =FOLDERS_COLOR from options. It's confusing and suggesting that You'd have to use = after -c. Make it simple.

This is standard syntax for bash programs. You don't have to use =, but you can, it's a valid syntax.
I've updated the handling of options to handle them in a more robust and unix-like way.
So you can call -c plasma, --color=plasma or --color plasma and it will work as expected.

Cf. http://docopt.org/

When used without any arguments it resets colour to plasma. Maybe it could just display help info?

Done.

Can You add empty line every piece of information?

This is already happening:

Folders color chooser

Current color: pistachio

Usage:
  folders-color-chooser [-c | --color=<color>] [-C | --reset-color]
  folders-color-chooser [-h | --help]
  folders-color-chooser [-l | --list]

Options:
  -c, --color=<color>   set the new folders color
  -C, --reset-color     reset folder color to plasma
  -h, --help            show this help
  -l, --list            list available colors

As you can see, I've added a -C (or --reset-color) option to revert to the default colour.

❯ ./scripts/folders-color-chooser -C
Changed from pistachio to plasma.

This is because there is no longer a default colour if the --color option is given without a value. Instead, we get a very clear error message.

❯ ./scripts/folders-color-chooser --color
Missing value for 'color' option. Please provide a color.

@SylEleuth
Copy link
Owner

I don't see spaces. I've just tested it on the fresh install on unmodified Konsole and it's still condensed.

❯ ./scripts/folders-color-chooser --color
Missing value for 'color' option. Please provide a color.

Maybe add available colours on the next line?

@akarzim
Copy link
Contributor Author

akarzim commented Jan 8, 2025

I don't see spaces. I've just tested it on the fresh install on unmodified Konsole and it's still condensed.

Hmm? I tried with both Gnome Terminal and Kitty, using zsh, bash & sh, and it works perfectly 😶

image

Which shell do you use?

❯ ./scripts/folders-color-chooser --color
Missing value for 'color' option. Please provide a color.

Maybe add available colours on the next line?

Good idea!

@SylEleuth
Copy link
Owner

Kitty:
image

Konsole (clean system install):
Screenshot_20250108_143908

@akarzim
Copy link
Contributor Author

akarzim commented Jan 8, 2025

I don't see spaces. I've just tested it on the fresh install on unmodified Konsole and it's still condensed.

I'm still not able to reproduce it. But I may have found an explanation in the echo manual:

NOTE: your shell may have its own version of echo, which usually supersedes the version described here.

So I replaced echo with printf for multiline output. Let me know if this prints the newlines as expected 🤞

@SylEleuth
Copy link
Owner

SylEleuth commented Jan 9, 2025

Fault was on my side. I've just copied the script because it was faster for me and empty lines have been omitted, but only there.

So add the colour options after empty -c argument and I think it will be all.

@akarzim
Copy link
Contributor Author

akarzim commented Jan 10, 2025

Fault was on my side. I've just copied the script because it was faster for me and empty lines have been omitted, but only there.

Okay, I can stop digging for weird bash behavior 😅

So add the colour options after empty -c argument and I think it will be all.

Done. 🥳

@SylEleuth SylEleuth merged commit 0ab8b68 into SylEleuth:master Jan 10, 2025
@SylEleuth
Copy link
Owner

Of course there were problems with pull, but eventually I resolved it somehow.
The issue: CONFLICT (add/add): Merge conflict in folders/olive/scalable/folder-olive-google-drive.svg. I had to git rebase --skip for every file (there were several of them). Any ideas?

Besides that the script is beautiful. It even works better than I could imagine. Thank You very much for doing it.

@akarzim
Copy link
Contributor Author

akarzim commented Jan 10, 2025

Of course there were problems with pull, but eventually I resolved it somehow.
The issue: CONFLICT (add/add): Merge conflict in folders/olive/scalable/folder-olive-google-drive.svg. I had to git rebase --skip for every file (there were several of them). Any ideas?

This is surprising. The diff only shows one file affected. There should be no conflict 🤔
Did you merge from the GitHub interface or from your command line? If the latter, what is the exact command you used?

@akarzim akarzim deleted the places-color-chooser branch January 10, 2025 13:07
@SylEleuth
Copy link
Owner

Command line:

git checkout -b ArtemChandragupta-master master
git pull git@github.com:akarzim/gruvbox-plus-icon-pack.git places-color-chooser --rebase --autostash

I've just merge other pull request and had the same adventure, some files were different.

@akarzim
Copy link
Contributor Author

akarzim commented Jan 10, 2025

Hmm 🤔 Why creating a new branch with git checkout -b ArtemChandragupta-master ?

I'd rather use these commands:

git switch master   # or `git checkout master`, it's equivalent
git fetch origin    # fetches changes remotely (if something has happened on the GitHub interface)
git pull --rebase   # realigns the local branch and the origin branch (master)
git pull git@github.com:akarzim/gruvbox-plus-icon-pack.git places-color-chooser --rebase --autostash
git push            # pushes pulled commits (or `git push origin master:master` to be verbose)

I hope this helps!

@SylEleuth
Copy link
Owner

I was following Github's instructions at the bottom of the pull request page. It looked like that:

git checkout akarzim-places-color-chooser
git pull git@github.com:akarzim/gruvbox-plus-icon-pack.git places-color-chooser
git checkout master
git merge --no-ff ArtemChandragupta-master
git push origin master

It worked for some time, and it broke at some point.
Your instruction are for a fork. What's a difference if I have a master and have to connect to a branch? I can't see that here.

@akarzim
Copy link
Contributor Author

akarzim commented Jan 10, 2025

These GitHub instructions seem odd to me.

git checkout akarzim-places-color-chooser     # switch to a non-existent branch, this should fail
git pull git@github.com:akarzim/gruvbox-plus-icon-pack.git places-color-chooser # ok
git checkout master                           # switch to master, without sync, some commits may be missing
git merge --no-ff ArtemChandragupta-master    # merges a completely different branch into master
git push origin master                        # at this stage, I don't know what you're pushing, but certainly not the expected changes :|

It might make sense to checkout akarzim/places-color-chooser with a slash to separate the remote from the branch. But in this case, you need to add a remote first:

git remote add akarzim git@github.com:akarzim/gruvbox-plus-icon-pack.git

Then you can run :

git fetch --all                                      # sync all remotes
git checkout --track akarzim/places-color-chooser    # switch to the remote branch
                                                     #   next time, just `checkout places-color-chooser`
git pull --rebase                                    # pull changes (if any)
git checkout master                                  # switch to master
git pull --rebase                                    # pull changes (if any)
git merge --no-ff akarzim/places-color-chooser       # merges each branch commit to master
git push origin master                               # push to origin remote

This method is more robust because it ensures that all the branches you're working on are up to date.

If you need to do the same thing for a branch of your own, simply ignore the ‘remote’ reference (ie. akarzim/) in the previous commands.

@SylEleuth
Copy link
Owner

My bad, missed -b in checkout command. Now I see that I messed up more things. I've just copied from terminal's history.

git checkout -b akarzim-places-color-chooser
git pull git@github.com:akarzim/gruvbox-plus-icon-pack.git places-color-chooser
git checkout master
git merge --no-ff akarzim-places-color-chooser
git push origin master

Does it look better now?

Next time I will use Your method. Or maybe I should just educate myself more on the subject (didn't expect that I have to since I thought I was the only one working on that project).

@akarzim
Copy link
Contributor Author

akarzim commented Jan 11, 2025

Does it look better now?

Yes, it does! Just to avoid misalignment, always do a git fetch --all before processing, and git pull --rebase after git checkout master 😉

@SylEleuth
Copy link
Owner

Will do. Thanks for the advice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants