Turn *_from_sheet into Result<Style>
Closed this issue · 3 comments
*_from_sheet is still fallible due to mounting. Currently it just panics which is a behaviour that might not be desirable.
I'd really prefer to instead pull mount
out of the constructors or offer a different constructor without mount that does not fail and document that in the respective _from_sheet
constructors.
I experimented with the code for #11 by delegating mounting to a StyleManager
, however, I realised that delayed mounting is causing complications on StyleRegistry.
I think first new
from either string or Sheet should be mounted by default.
As most of the time, this is the behaviour you want.
Currently, Style::new
does the following:
- Parse css into Sheet if not sheet
- Register with cache and if exist in cache return cached instance
- Mount the style
If a Sheet
is passed to an infallible method (say: new_from_sheet_unmounted
), what does it do?
I think here neither 1 nor 3 applies. So the only thing it would do is 2. That is add / retrieve it to / from cache.
Consider the following code:
let sheet: Sheet = "color: red;".parse()?;
let style_a = Style::new_from_sheet_unmounted(sheet);
// ...
let style_b = Style::new("color: red;")?;
// ...
style_a.mount()?;
Are style_a
and style_b
the same Style instance in StyleRegistry
?
- If yes, will the Style be mounted when
style_b
is created?- If so, then the Style needs to be mounted at the time
style_b
is created. So.mount()
does not guarantee a mount. what's advantage of having an unmounted Style over holding a Sheet than simply holdingsheet
and delaystyle_a
's instantiate until it mounts? Given that you still need to hold aStyle
instance to call the.mount()
method. This design also will not allow a custom mounting point as a style can only be mounted once.
- If so, then the Style needs to be mounted at the time
- If no, then
style_a
andstyle_b
will be treated as different styles. How will theStyleKey
look like?
Do we key by elements? If so how to implementPartialEq
andEq
andHash
for StyleKey? Especially given that it changes depending on mounting status.
In addition, asweb_sys::Element
is!Sync
and!Send
, this would cause the currentArc<Mutex<X>>
scheme to be unusable on theStyleRegistry
unless weunsafe impl
. I guess this is solvable by turning Registry to be thread local and haveRc<RefCell<X>>
instead, but I didn't bother to experiment as that requires an additional dependency.
Furthermore, theStyleRegistry
is created to deduplicate mounting (and parsing in the past). Which kind of defects the purpose of it if it cannot reduce mounting effectively.
With all these complications delayed mounting is going to create, is it worth to make such a change (and potentially introduce unsafe
into the implementation) to cater a niche behaviour, especially given that delaying Style::new
until it's ready to be mounted can avoid all the situation above?
Without delayed mounting, the manager API can be as easy as:
let mgr = IframeManager::with_selector("#my_frame"); // No holding of Element at the moment, because of `!Send` and `!Sync`.
// Each Manager also has it's own Registry, so it won't affect Styles mounted under other managers.
let sheet: Sheet = "color: red;".parse()?;
let style_b = Style::new_with_manager("color: red;", mgr.clone())?; // Inner of Managers should be `Arc`'ed, so cloning here is as cheap as cloning an Arc.
// ...
let style_a = Style::new_with_manager(sheet, mgr.clone())?;
If yes, will the Style be mounted when
style_b
is created?* If so, then the Style needs to be mounted at the time `style_b` is created. So `.mount()` does not guarantee a mount. what's advantage of having an unmounted Style over holding a Sheet than simply holding `sheet` and delay `style_a`'s instantiate until it mounts? Given that you still need to hold a `Style` instance to call the `.mount()` method. This design also will not allow a custom mounting point as a style can only be mounted once.
I would express mount/unmount
as:
{precondition: Style is mounted or unmounted} mount() {postcondition: Style is mounted}
. The Style gains one user.{precondition: Style is mounted} unmount() {postcondition: Style might still be mounted by other users}
The Style loses one users.
That is, mounting a Style guarantees that afterwards the Style is present and ready for use in html. Unmount tells stylist
that it won't be needed until it's mounted again, but there might be other mounts of the same style still requiring it to stay mounted. Each manager for its own sounds like a good idea as well 👍