feat: theme integration #196
Conversation
wash2
force-pushed
the
theme-integration_jammy
branch
from
831bed5 to
4df82c3
Compare
There was a problem hiding this comment.
I am a little torn reviewing this..
- Either locking isn't cheap and we want to avoid the runtime cost. Then every element should cache the theme after creation. One-off elements like the
IndicatorShader-elements should instead have a cached theme (or parts of it) in their parent (soTilingLayoutfor example). In those cases we would only read from the global theme at startup and when it gets updated. - Or locking is negligible and we could just access the global state everywhere. Then we don't need
set_themeon theCosmic*-elements, as they could just access the theme directly whenever they need it.
Honestly I would love to have the best of both worlds. No locking, so it would probably need to be stored in State/Common, but passed by argument, so we don't have caching. But the TilingLayout needs gaps for arrange, which is called as part of almost every method, so I get, that we have to cache some parts here.
So sorry for requesting quite a refactor, but thinking about this, I would prefer:
- The
Themeto be part of theCommon-struct, so that it can be accessed throughstate.common.theme, instead of as a global - The theme should be passed through the
render_outputfunctions, so that it is readable when creating theRenderElements. - The only non-color property is the
gaps-property and theTilingLayoutshould indeed cache that. - The
IcedElementalso needs the theme outside ofrender_elements, since for some reasonupdateneeds it... So we also keep a copy there and initialize that onIcedElement::new. This means, that allCosmic*-elements also need a new theme argument on creation. This might be the most annoying consequence of this design, but I already need anEventLoopHandlebecause of the async-runtime required for IcedElements. SoStateshould already be available in some form in all places to get an event-loop handle and the theme could be cached/stored/cloned the same way. - To update gaps and iced-elements, we introduce
shell.update_theme(&Theme), which can callupdate_gapsfor all workspaces/tiling_layout andreload_themefor allmapped-elements.
I hope I didn't miss anything, that caused you to go the global-variable route instead, but to me this seems quite sound. If you feel stuck, I can also finish this off, since I have a pretty good idea, how I would like to see this integrated. But should you want to tackle it, I hope the idea has become clear.
Also in case this wasn't clear: Thanks for all the effort that already went into this. I am very much looking forward to this feature and I am quite happy I have not had to figure all of this out on my own.
| @@ -55,6 +56,8 @@ fn main() -> Result<()> { | |||
| // potentially tell the session we are setup now | |||
| session::setup_socket(event_loop.handle(), &state)?; | |||
|
|
|||
| let _theme_res = theme::watch_theme(event_loop.handle()); | |||
There was a problem hiding this comment.
| let _theme_res = theme::watch_theme(event_loop.handle()); | |
| if let Err(err) = theme::watch_theme(event_loop.handle()) { | |
| warn!(?err, "Failed to watch cosmic theme"); | |
| } |
| let theme_guard = THEME.read().unwrap(); | ||
| let theme = theme_guard.clone(); | ||
| drop(theme_guard); |
There was a problem hiding this comment.
Can't this simply be?
| let theme_guard = THEME.read().unwrap(); | |
| let theme = theme_guard.clone(); | |
| drop(theme_guard); | |
| let theme = THEME.read().unwrap().clone(); |
| let mut internal = IcedElementInternal { | ||
| outputs: HashSet::new(), | ||
| buffers: HashMap::new(), | ||
| pending_update: None, | ||
| size, | ||
| cursor_pos: None, | ||
| theme: Theme::dark(), // TODO | ||
| theme: theme.clone(), // TODO |
There was a problem hiding this comment.
| theme: theme.clone(), // TODO | |
| theme: theme.clone(), |
| @@ -759,6 +759,22 @@ impl CosmicMapped { | |||
| popup_elements.into_iter().map(C::from).collect(), | |||
| ) | |||
| } | |||
|
|
|||
| pub(crate) fn set_theme(&self) { | |||
There was a problem hiding this comment.
Since this has no parameter, could we maybe rename this to something like reload_theme or update_theme? (Same then for CosmicStack, CosmicWindow, IcedElement..)
|
|
||
| pub(crate) fn update_gaps(&mut self, gaps: (i32, i32)) { | ||
| self.gaps = gaps; | ||
| } |
There was a problem hiding this comment.
This is somewhat inconsistent. The IcedElement reloads the theme, but although theme::gaps() exists, this gets set instead of a similar update_gaps-function. (Or more generally update_theme.)
Not sure what solution would be cleanest, but I would like it to be consistent.
| @@ -4086,7 +4088,7 @@ where | |||
| if render_active_child { 16 } else { 8 }, | |||
| alpha * if render_potential_group { 0.40 } else { 1.0 }, | |||
| output_scale, | |||
| GROUP_COLOR, | |||
| group_color(), | |||
There was a problem hiding this comment.
So the IcedElement caches the theme, while the render_output function of the TilingLayout just locks the THEME many times through *_color-functions? (See overall review comment.)
|
Ok thanks for the review. I'll try refactoring this as you've suggested and let you know if I run into any additional issues. |
Depends on pop-os/libcosmic#178 and should make cosmic-comp theme variables customizable via cosmic-settings after pop-os/cosmic-settings#78