-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
client/src/main.rs
Outdated
} | ||
} | ||
} | ||
|
||
fn hexcolor(color: &str) -> iced::Color { | ||
let hex_col = HexColor::parse(color).unwrap(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :).
Co-authored-by: Christian Friedow <[email protected]>
Good job @pinpox! You can run the formatter toolchain in the project with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also a few color values still hardcoded which should be changed as well.
text:
- https://github.com/friedow/centerpiece/blob/main/client/src/component/entry.rs#L48
- https://github.com/friedow/centerpiece/blob/main/client/src/component/query_input.rs#L56
surface:
Co-authored-by: Christian Friedow <[email protected]>
Co-authored-by: Christian Friedow <[email protected]>
@friedow Updated the missing colors. I also moved the Maybe it would make sense to apply the |
You'll need to run the formatter again :) |
## 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
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.