Skip to content

transparent StackView layers #254

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

Merged
merged 1 commit into from
Jun 7, 2018
Merged

transparent StackView layers #254

merged 1 commit into from
Jun 7, 2018

Conversation

dermetfan
Copy link
Contributor

@dermetfan dermetfan commented May 31, 2018

In response to #117 (comment) I removed the Layer from StackView, added a background to Dialog and Panel, and adapted usages. On the caller's side that leads to the following situation:

add_layer(Layer::new(…))

By default there will be no background so in its current state this silently breaks many applications and is not very intuitive / easy to do wrong. You can see how many occurrences I had to update in the examples. Because the background is not cleared this also causes artifacts when scrolling text.

Maybe we should keep the default behavior and just add something like add_transparent_layer()? I'd like to agree on the API changes before I go ahead.

@gyscos
Copy link
Owner

gyscos commented Jun 1, 2018

Thank you very much for the work!

You're right, silently changing add_layer to be transparent might be surprising; a add_transparent_layer sounds like a better solution.

A simple solution would be, from your PR, to simply rename add_layer to add_layer_transparent, and add something like

fn add_layer<T>(&mut self, view: T)
    where T: IntoBoxedView,
{
    self.add_transparent_layer(Layer::new(view));
}

Unfortunately, thinking about it more, we have the same problem we had with the optional ShadowView wrapper: we want users who pop layers to get the View they inserted. So if I do add_layer(MyView::new()), I want pop_layer to return this MyView, without being wrapped by a Layer or ShadowView.

The solution is the same we had back then: add a variant to the ChildWrapper enum, so that it could be either Shadow(ShadowView<Layer<T>>), Backfilled(Layer<T>), or Transparent(T).

This means adding add_transparent_layer and add_transparent_layer_at.

This may sound like an uncontrolled explosion in methods or variants, but:

  • The variants for ChildWrapper are bounded: we did not add a new wrapper, we removed the last mandatory wrapper.
  • We could remove add_layer_at and only keep add_transparent_layer_at: it's an advanced API to give more control; we can expect users in this case to be fine with manually wrapping a view in a Layer.

Going this road, I don't think it makes sense to backfill in Dialogs or Panels anymore.

@dermetfan
Copy link
Contributor Author

dermetfan commented Jun 3, 2018

That turned out pretty well, only StackView is modified now. Thanks for the input!
I also added transparent_layer() for a chainable variant.

// Some views don't (fullscreen views mostly)
Plain(Layer<T>),
Plain(T),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Kept the Plain name for now, I think it's still a good fit)

@dermetfan dermetfan changed the title [WIP] transparent StackView layers transparent StackView layers Jun 3, 2018
@@ -77,18 +83,22 @@ impl<T: View> ChildWrapper<T> {
/// Returns a reference to the inner view
pub fn get_inner(&self) -> &View {
match *self {
ChildWrapper::Shadow(ref shadow) => shadow.get_inner().get_inner(),
ChildWrapper::Plain(ref layer) => layer.get_inner(),
ChildWrapper::Shadow(ref shadow) => shadow.get_inner(),
Copy link
Owner

Choose a reason for hiding this comment

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

I think we still want the absolute inner view here, so it would be get_inner().get_inner() (just like before this PR)?

}
}

/// Returns a mutable reference to the inner view
pub fn get_inner_mut(&mut self) -> &mut View {
match *self {
ChildWrapper::Shadow(ref mut shadow) => {
shadow.get_inner_mut().get_inner_mut()
shadow.get_inner_mut()
Copy link
Owner

Choose a reason for hiding this comment

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

Same thing here, any reason you removed a round of get_inner_mut()?

Copy link
Contributor Author

@dermetfan dermetfan Jun 6, 2018

Choose a reason for hiding this comment

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

No, those are my fault, it seems I haven't properly restored everything after the previous attempt.

@dermetfan
Copy link
Contributor Author

Good catch! I added a test that would have caught my mistake.

test StackView::get()
@gyscos
Copy link
Owner

gyscos commented Jun 7, 2018

Awesome, thanks!

@gyscos gyscos merged commit d31903c into gyscos:master Jun 7, 2018
@dermetfan dermetfan deleted the bg-layer branch June 7, 2018 09:06
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.

2 participants