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

Allow setting colors via configuration #141

Merged
merged 12 commits into from
May 23, 2024
Merged

Allow setting colors via configuration #141

merged 12 commits into from
May 23, 2024

Conversation

pinpox
Copy link
Contributor

@pinpox pinpox commented May 16, 2024

Partly Fixes #108

I implemented basic color settings trying to follow the other settings. Since I havn't worked with this very much before, I'm sure there is stuff I missed or should be done differently. Please let me know what has to change, any hints very welcome.

I only implemented colors for now. Not sure if font settings should be a separate PR.

client/Cargo.toml Outdated Show resolved Hide resolved
client/src/settings.rs Show resolved Hide resolved
client/src/settings.rs Outdated Show resolved Hide resolved
client/src/settings.rs Outdated Show resolved Hide resolved
client/src/settings.rs Outdated Show resolved Hide resolved
}
}
}

fn hexcolor(color: &str) -> iced::Color {
let hex_col = HexColor::parse(color).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unwrapping this here is an unsafe action which could terminate the application. It is fair to terminate the application in case of a wrong configuration. However, this should happen at startup time and with a proper error message.

Please add a proper error message if the parsing failed before unwrapping this value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@friedow Thanks for the review! I added an error message here, there is still an issue though (marked with "TODO") in this function: The alpha value of the hex code is not used, because I couldn't figure out how to convert it correctly. Any ideas?

Also, is the error message enough or should I do anything else so that the parsing happens at startup?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! No need to adjust this further.

My best guess right now is try: hex_col.a as f32. I can't try it myself right now because I'm on holidays and don't have a laptop on me 😅. If that does not work I can try to help with that on Monday :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hex_col.a as f32 works, I just pushed the change. I'm not sure though it does the correct thing, the parameter seems to have little if any effect. Test it when you find the time after monday and let me know if anything else needs to change.

Also we should maybe set a better default color scheme via the default colors, the ones I set currently are for testing purpouses.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I just thought about this again. The conversion does what it's supposed and we don't get a compilation error anymore because the types are correct. But the hex_col.a value is probably on a scale from 0-255 and the iced color wants an alpha value on a scale of 0-1.
I'll check the docs on this on Monday and will also test this :).

@friedow friedow self-assigned this May 17, 2024
@a-kenji
Copy link
Collaborator

a-kenji commented May 21, 2024

Good job @pinpox! You can run the formatter toolchain in the project with:

nix fmt

@pinpox pinpox marked this pull request as ready for review May 21, 2024 21:49
Copy link
Owner

@friedow friedow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client/src/main.rs Outdated Show resolved Hide resolved
client/src/settings.rs Outdated Show resolved Hide resolved
@pinpox
Copy link
Contributor Author

pinpox commented May 23, 2024

@friedow Updated the missing colors. I also moved the hexcolor helper function to settings.rs so I don't have to define it every time. Let me know if that is not the right place for it.

Maybe it would make sense to apply the hexcolor() just one time on startup when reading all the settings form the file and have the color settings be a global that is already of the type iced::Color and can referenced?

@friedow
Copy link
Owner

friedow commented May 23, 2024

You'll need to run the formatter again :)

@friedow
Copy link
Owner

friedow commented May 23, 2024

You're absolutely right, the settings struct should become a singleton. I've created a ticket for that (#142). Feel free to pick that up if you want :).

Anyways, this PR is read to be merged now 🎉! Thank you again for this great contribution @pinpox!

@friedow friedow merged commit 008c3de into friedow:main May 23, 2024
1 check passed
@friedow friedow mentioned this pull request Jun 2, 2024
friedow added a commit that referenced this pull request Jun 2, 2024
## Release Notes

### ✨ New Features
* 💄 display bars to visualize cpu core usage by @friedow in
#121
* ✨ `git_repositories`: add `zoxide` integration by @a-kenji in
#111
* ✨ enable font-fallack for all entries by @friedow in
#131
* 🌈 Allow setting colors via configuration by @pinpox in
#141
* 😜 Gitmoji plugin by @friedow in
#144
* 📑 Firefox bookmarks plugin by @friedow in
#133
* 📜 Firefox History by @friedow in
#152

### 🐛 Fixed Bugs
* 🐛 return false when file_name cannot convert to str by @prmadev in
#123
* 🐛 filter empty plugins for calculating scroll offset by @friedow in
#125
* 🐛 Add sort function to plugin trait by @a-kenji in
#124
* 🐛 implement full desktop spec by @friedow in
#120
* 🐛 fix initial sorting in a bunch of plugins by @friedow in
#129
* 🐛: fix suspend command invocation by @a-kenji in
#134

### Maintenance
* 📝 clarify wayland support by @a-kenji in
#105
* 👷 remove magic-nix-cache action by @a-kenji in
#106
* ✏️ Fix a small typo in the Readme by @a-kenji in
#112
* 🚨 Implement various clippy suggestions by @a-kenji in
#113

### New Contributors
* @prmadev made their first contribution in
#123
* @pinpox made their first contribution in
#141

**Full Changelog**:
v1.0.0...v1.1.0
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.

Feature request: custom colors and fonts
3 participants