-
Notifications
You must be signed in to change notification settings - Fork 126
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
Create a minimal xilem_masonry #205
Conversation
Some of these suggestions are kind of bad but that's by-the-by
} | ||
} | ||
|
||
impl<State, Logic, View> Xilem<State, Logic, View> |
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.
This might be too cute, but I'd like to keep it like this
I wanted to avoid the term App
, because this is plausibly embeddable.
/// | ||
/// This includes only the widgets which might send actions | ||
/// This is currently never cleaned up | ||
widget_map: HashMap<WidgetId, Vec<ViewId>>, |
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.
I believe we expect Masonry to do internal tree structure tracking at some point, which I think would include widget deleted events?
I don't know a clean way to tidy this up, otherwise
/// | ||
/// These need to be public for type inference | ||
#[doc(hidden)] | ||
pub struct WasAView; |
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.
This avoids needing ViewMarker
} | ||
} | ||
|
||
// TODO: We use raw indexing for this value. What would make it invalid? |
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.
The design of ViewSequence
here is subtly different to previous versions of Xilem.
Currently, each non-trivial ViewSequence
s add a level to the ViewId path, and they have complete control of their own ViewId
.
This is a quite different, much sparser model of view ids
} | ||
|
||
fn mutate(&mut self) -> WidgetMut<Box<dyn masonry::Widget>> { | ||
unreachable!("VecSplice can only be used for `build`, not rebuild") |
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.
This is an interesting one. I suspect that ViewSequence::build
might want to take an append only Vec
, and so only mutate takes an ElementSplice
.
|
||
use crate::{ElementSplice, MasonryView, VecSplice, ViewSequence}; | ||
|
||
// TODO: Allow configuring flex properties. I think this actually needs its own view trait? |
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.
These kinds of specialised view traits would also be the idiomatic way to create and manage the properties of winit Window
s, for example (in addition to menus?)
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.
cargo run --package xilem_masonry --example mason
xilem_masonry_flex.webm
@@ -105,4 +105,5 @@ jobs: | |||
uses: Swatinem/rust-cache@v2 | |||
|
|||
- name: cargo doc | |||
run: cargo doc --workspace --all-features --no-deps --document-private-items -Zunstable-options -Zrustdoc-scrape-examples | |||
# We currently skip checking masonry's docs |
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.
We're probably going to need to make an issue for this
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.
LGTM!
} | ||
} | ||
|
||
const BASE_ID: NonZeroU64 = match NonZeroU64::new(1) { |
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.
Could be unwrap, I think?
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.
unwrap isn't const.
ViewId
being NonZero
is a little bit weird at the moment, so it might be worth reconsidering.
Alternative to #218 The version of viewsequence added in #205 is robust for all properties supported in that version. However, it would be incompatible with future expansions to the Xilem model, in particular async wiring. In this PR, I propose a system to resolve this, which uses generational indexes in the view id path (for the items where this is relevant). Some other historical record: #9 This brings back the `View::State` associated type (renamed to `ViewState` and `SeqState` depending on the trait), in order to perform this wiring.
Alternative to #204
Demonstrates a viable path forward to Xilem. This does not get rid of the widget set in the main Xilem crate, or migrate the main Xilem crate to this