hyprland-community / hyprland-autoname-workspaces

Hyprland autoname workspaces 🪟 [maintainers=@cyrinux,@maximbaz]
ISC License
199 stars 7 forks source link

feat: make icon delimiter modifiable in config #47

Closed psimovec closed 1 year ago

psimovec commented 1 year ago

Description

This PR makes the delimiter between icons modifiable in config file, instead of using forced space. It can be modified by setting delim="something" in config.

Examples: delim=" - " image delim=" " #SPACE image delim=" " # NARROW NO-BREAK SPACE image Someone might prefer no delimiter, or possibly set background color/background color of the delimiter delim=" <span bgcolor='white'> </span> " image (requires some patches for hyprland-rs and waybar-hyprland)

I am VERY open to feedback, it is my very first piece of code written in Rust and I have basically no knowledge about this programming language.

Type of change

How Has This Been Tested?

No extensive testing, I have just tried modifying the delim option., (I should write at least 1 test)

Future

In next PR/PRs I would like to make whole "workspace pattern" modifiable in config, specifically this: let text = format!("{id}: {apps}");

And then the same for general "icon"/"icon with counter".. So it is possible to format it as people wish, for example setting a different color for the counter of icons.

cyrinux commented 1 year ago

Congrats @psimovec, very good start, I'm learning too with this project. I will fetch locally and test, maybe we can remove some clone(). Otherwise I'm agree for the idea of using formatter in more intentisive way too.

hint: Abuse of cargo clippy and cargo build to help you cleaning your code.

psimovec commented 1 year ago

Thanks for the review, it looks better now. (I'd squash commits + possibly rebase if needed when we decide the PR is ready to be merged, so i don't force push 5 times a day :D)

psimovec commented 1 year ago

@cyrinux do you prefer this PR as 1 commit or do you prefer 2 commits - one for code, one for docs?

cyrinux commented 1 year ago

Please try to add the [format] config level and put the delim under, if you can't find how to do it, tell me, and I will do it after you. 🙏🏻

cyrinux commented 1 year ago

@cyrinux do you prefer this PR as 1 commit or do you prefer 2 commits - one for code, one for docs?

As you wish, but don't worry with number of commits, we can rebase at the end before get merge to include the feature. I would prefer merge add the end one commit with the whole stuff for the PR.

psimovec commented 1 year ago

Please try to add the [format] config level and put the delim under, if you can't find how to do it, tell me, and I will do it after you. 🙏🏻

Should dedup be under the [format] too?

cyrinux commented 1 year ago

Please try to add the [format] config level and put the delim under, if you can't find how to do it, tell me, and I will do it after you. 🙏🏻

Should dedup be under the [format] too?

I was not sure about it but if you ask me, I feel like we should. Put it under format section as this all config will be under a section.

cyrinux commented 1 year ago

Another hint, if it is not already the case, use a code editor with the rust-analyzer lsp extension/config enable this will help.

psimovec commented 1 year ago

I am struggling to initialize the FxHashMap to have default delim=" ", I commented it out for now I'll try to install the extension you mentioned

psimovec commented 1 year ago

Alright even chatgpt3 is way better programming than me, it solved the problem with default delim for me, code doesn't look that nice but it seems to work :D

cyrinux commented 1 year ago

Alright even chatgpt3 is way better programming than me, it solved the problem with default delim for me, code doesn't look that nice but it seems to work :D

Let me use chatgpt4 to try to remove the mut 😄

psimovec commented 1 year ago

Let me use chatgpt4 to try to remove the mut smile

I guess time to buy chatGPT subscription again :D

cyrinux commented 1 year ago

I am struggling to initialize the FxHashMap to have default delim=" ", I commented it out for now I'll try to install the extension you mentioned

We/(you) can try to move dedup under format now.

psimovec commented 1 year ago

I kinda did it, but I don't like the way I did it, right now you would have to use something like dedup="true" which is really weird.. Not sure if I will have more time today+tomorrow, so I am totally fine if you modify this PR as you wish/add your changes so it looks/works better

psimovec commented 1 year ago

probably it will be the best to remove last commit https://github.com/hyprland-community/hyprland-autoname-workspaces/pull/47/commits/b2f4b65ff33edd2ab2dffd66dfacbd4b0eb09dc4 and to do it properly

cyrinux commented 1 year ago

Ok I will try to progress on this top of your commit. It's already a good progress!

cyrinux commented 1 year ago

@psimovec if you want I try to do it from my side then I guide you further, when I will have a better view of what we target, when you can. If you really want I make it and you will read then, tell me.

cyrinux commented 1 year ago

@cyrinux do you prefer this PR as 1 commit or do you prefer 2 commits - one for code, one for docs?

As you wish, but don't worry with number of commits, we can rebase at the end before get merge to include the feature. I would prefer merge add the end one commit with the whole stuff for the PR.

Our co-maintainer @maximbaz prefer we dont squash or rebase, so feel free to just commit each of your steps for the future 😉

cyrinux commented 1 year ago

@psimovec I merge this PR that superseed this one, I make my work on top of yours https://github.com/hyprland-community/hyprland-autoname-workspaces/pull/49 You can try the formatters in the master branch now ! 😄