-
Notifications
You must be signed in to change notification settings - Fork 323
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
New selection box implementation for component groups #3520
Conversation
@farmaazon all comments addressed. |
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.
One of my comments in the previous PR seems unaddressed: the one about not displaying selected_label when there is no selection layer.
fn set_strong_color(&self, color: color::Rgba) { | ||
if let Some(shape) = &self.shape { | ||
shape.strong_color.set(color.into()); | ||
} | ||
} | ||
|
||
fn set_weak_color(&self, color: color::Rgba) { | ||
if let Some(shape) = &self.shape { | ||
shape.weak_color.set(color.into()); | ||
} |
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.
"strong" and "weak" color sounds strange. Let's name it "active_color" and "inactive_color" instead.
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.
They are not active
or inactive
, they are used by icons with multiple colored parts involved. This is described in the docs of icon::Any
(this shape variable), I've also added it to the docs of the Colors
struct.
//! So instead we duplicate component group entries in a special `selection` scene layer (or | ||
//! rather, in a multiple scene layers to ensure render ordering) and use [layers masking][mask] | ||
//! to cut off everything except the selection shape. We duplicate the following: | ||
//! - Entry text and icon (see [entry][] module). | ||
//! - A background of the component group [background][]. | ||
//! - (for component groups with header) A background of the header [selection_header_background][]. |
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.
Please use some grammar-fixing tool. There is a lot of places with missing commas and small grammar issues. Apply to all doc comments, please.
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.
Done
// === Selection === | ||
|
||
/// A shape of selection box. It is used as a mask to show only specific parts of the selection | ||
/// layers. See module-level documentation. |
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.
See module-level documentation.
-> See module-level documentation to learn more.
. Please apply to other docs as well.
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.
Funny, I had your variant in the first version of the code, and then I thought it was too long.
let params = | ||
entry::Params { colors: colors.clone_ref(), selection_layer: default() }; |
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 prefer 2 expressions with 2 variables than one multiline expression. Please, refactor colors
to a separate variable, and make it one-liner.
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.
Done
pub fn new(logger: &Logger, normal_parent: &Layer, selected_parent: &Layer) -> Self { | ||
let normal = LayersInner::new(logger, normal_parent); | ||
let selection = LayersInner::new(logger, selected_parent); | ||
|
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.
why this newline?
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.
Removed
let params = | ||
entry::Params { colors: colors.clone_ref(), selection_layer: default() }; |
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.
multi-line exprs. Please apply to other places in the code.
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.
Done. There are a few places with multiline expressions in the code, but it's impossible to change it without making the code way uglier.
/// The selection animation is faster than default one because of the increased spring force. | ||
const SELECTION_ANIMATION_SPRING_FORCE: f32 = 30_000.0; |
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.
Leave here a multiplier intead. So if we change the spring force globally thsi will update automatically.
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.
Done
/// This is an example code to position the selection box on scene. | ||
/// `multiview.selection_position_target` returns a group-local position, so we transform it | ||
/// to global one and then restrict the y coordinate so that the selection box would not go | ||
/// below the scroll area bottom. |
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.
It's really hard to read these sentences. Please make them shorter and run them trough a grammar checking tool. Gramarly is cool. If you need pro license, Ola will buy you one.
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.
Done.
It should fix a spotted bug when selecting entries in the wide component group
QA acceptance passed, merging it now. |
[Task link](https://www.pivotaltracker.com/story/show/182194574). [ci no changelog needed] This PR implements a new selection box that will replace an old (not really working) one in the component browser. The old selection box wasn't working well with the headers of the component groups, so we were forced to make a much harder implementation. The new implementation duplicates some visual components and places them in a separate layer. Then, a rectangular mask cuts off everything that is not "selected". This way: - We have more control over what the selected entries should look like. - We can easily support the multi-layer structure of the component groups with headers. - We avoid problems with nested masks that our renderer doesn't support at the moment. To be more precise, we duplicate the following: - Background of the component group becomes the "fill" of the selection. - Entries text and icons - we can alter them easily. - Header background and header text. By placing them in separate scene layers we ensure correct rendering order. https://user-images.githubusercontent.com/6566674/173657899-1067f538-4329-44f9-9dc2-78c8a4708b5a.mp4 # Important Notes - This PR implements the base of our future selection mechanism, selecting entries with a mouse and keyboard still has several issues that will be fixed in the future tasks. - The scrolling behavior will also be improved in future tasks. Right we only restrict the selection box position so that it never leaves the borders of the component group. - I added a new function to `add` shapes to new layers in a non-exclusive way (we had only `add_exclusive`) before. I have no idea how we didn't use this feature before even though we mention it a lot in the docs. - The demo scene restricts the position of the selection box for one-column component groups but does not for the wide component group.
[Task link](https://www.pivotaltracker.com/story/show/182194574). [ci no changelog needed] This PR implements a new selection box that will replace an old (not really working) one in the component browser. The old selection box wasn't working well with the headers of the component groups, so we were forced to make a much harder implementation. The new implementation duplicates some visual components and places them in a separate layer. Then, a rectangular mask cuts off everything that is not "selected". This way: - We have more control over what the selected entries should look like. - We can easily support the multi-layer structure of the component groups with headers. - We avoid problems with nested masks that our renderer doesn't support at the moment. To be more precise, we duplicate the following: - Background of the component group becomes the "fill" of the selection. - Entries text and icons - we can alter them easily. - Header background and header text. By placing them in separate scene layers we ensure correct rendering order. https://user-images.githubusercontent.com/6566674/173657899-1067f538-4329-44f9-9dc2-78c8a4708b5a.mp4 # Important Notes - This PR implements the base of our future selection mechanism, selecting entries with a mouse and keyboard still has several issues that will be fixed in the future tasks. - The scrolling behavior will also be improved in future tasks. Right we only restrict the selection box position so that it never leaves the borders of the component group. - I added a new function to `add` shapes to new layers in a non-exclusive way (we had only `add_exclusive`) before. I have no idea how we didn't use this feature before even though we mention it a lot in the docs. - The demo scene restricts the position of the selection box for one-column component groups but does not for the wide component group.
[Task link](https://www.pivotaltracker.com/story/show/182194574). [ci no changelog needed] This PR implements a new selection box that will replace an old (not really working) one in the component browser. The old selection box wasn't working well with the headers of the component groups, so we were forced to make a much harder implementation. The new implementation duplicates some visual components and places them in a separate layer. Then, a rectangular mask cuts off everything that is not "selected". This way: - We have more control over what the selected entries should look like. - We can easily support the multi-layer structure of the component groups with headers. - We avoid problems with nested masks that our renderer doesn't support at the moment. To be more precise, we duplicate the following: - Background of the component group becomes the "fill" of the selection. - Entries text and icons - we can alter them easily. - Header background and header text. By placing them in separate scene layers we ensure correct rendering order. https://user-images.githubusercontent.com/6566674/173657899-1067f538-4329-44f9-9dc2-78c8a4708b5a.mp4 # Important Notes - This PR implements the base of our future selection mechanism, selecting entries with a mouse and keyboard still has several issues that will be fixed in the future tasks. - The scrolling behavior will also be improved in future tasks. Right we only restrict the selection box position so that it never leaves the borders of the component group. - I added a new function to `add` shapes to new layers in a non-exclusive way (we had only `add_exclusive`) before. I have no idea how we didn't use this feature before even though we mention it a lot in the docs. - The demo scene restricts the position of the selection box for one-column component groups but does not for the wide component group.
Pull Request Description
Task link.
[ci no changelog needed]
This PR implements a new selection box that will replace an old (not really working) one in the component browser. The old selection box wasn't working well with the headers of the component groups, so we were forced to make a much harder implementation.
The new implementation duplicates some visual components and places them in a separate layer. Then, a rectangular mask cuts off everything that is not "selected". This way:
To be more precise, we duplicate the following:
2022-06-14.20-58-48.mp4
Important Notes
add
shapes to new layers in a non-exclusive way (we had onlyadd_exclusive
) before. I have no idea how we didn't use this feature before even though we mention it a lot in the docs.Checklist
Please include the following checklist in your PR:
Scala,
Java,
and
Rust
style guides.
./run ide dist
and./run ide watch
.