Grid View with Scrolling #3588
Grid View with Scrolling #3588
Conversation
…-view-simple-182585402
…-view-simple-182585402
farmaazon
requested review from
MichaelMauderer,
4e6,
mwu-tow,
kazcw,
PabloBuchu and
jdunkerley
as code owners
| /// An internal structure of [`Entry`], which may be passed to FRP network without risk of memory | ||
| /// leaks. |
There was a problem hiding this comment.
- I don't understand this comment. Why there could be a risk of a memory leak. Could you elaborate more in the comment, please?
- There are English language issues in the doc comments (here and in other doc comments). Could you use an English listing software for the comments, please?
There was a problem hiding this comment.
Ad 1. I mean: it may be passed to FRP network in contrary to the entire Entry structure, which contains Network. I removed the second part - I think "may be passed to FRP network" is enough.
|
|
||
| impl EntryData { | ||
| fn new(app: &Application, text_layer: &Option<Layer>) -> Self { | ||
| let logger = Logger::new("list_view::entry::Label"); |
There was a problem hiding this comment.
Use the new logging that is exposed in prelude instead of our old logger, please.
There was a problem hiding this comment.
I have to pass something to display::object::Instance. Can I do it in the new logging somehow?
| data: Rc<EntryData>, | ||
| } | ||
|
|
||
| impl crate::Entry for Entry { |
There was a problem hiding this comment.
why do you need to do crate::Entry in one place but not in the other place (both are in the same line).
There was a problem hiding this comment.
Because those are different entities. One is crate::Entry and the second is basic::Entry. One is a general "Entry" trait, the second is a concrete implementation.
| bg_color <- input.set_params.map(|params| params.bg_color).on_change(); | ||
| bg_margin <- input.set_params.map(|params| params.bg_margin).on_change(); | ||
| font <- input.set_params.map(|params| params.font.clone_ref()).on_change(); | ||
| text_offset <- input.set_params.map(|params| params.text_offset).on_change(); | ||
| text_color <- input.set_params.map(|params| params.text_color).on_change(); | ||
| text_size <- input.set_params.map(|params| params.text_size).on_change(); |
There was a problem hiding this comment.
here we fit the line, so it's not so important, but in such cases as these, I normally prefer to write |t| t.bg_color instead of |params| params.bg_color because the previous word already indicates what we are dealing with (set_params.map). Again, not important at all, but shorter code is easier to parsing with eyes.
| &self.data.display_object | ||
| } | ||
| } | ||
|
|
| /// **Important**. The [`Frp::model_for_entry_needed`] are emitted once when needed and not repeated | ||
| /// anymore, after adding connections to this FRP node in particular. Therefore make sure, that you | ||
| /// connect providing models logic before emitting any of [`Frp::set_entries_size`] or | ||
| /// [`Frp::set_viewport`]. |
There was a problem hiding this comment.
Are you remembering all the data that was transmitted to you? If so, could you add a limit for it and remove old data? I'm just afraid that our memory consumption can blow up trough the window otherwise with big tables of data. What do you think? Maybethe worry is not needed now.
There was a problem hiding this comment.
No, the data are passed to visible entries. We don't cache them. When the view is scrolled back, we re-request them. I can add info there.
| /// arguments, similar to [`crate::GridViewTemplate`] - see its docs for details. | ||
| #[derive(CloneRef, Debug, Deref, Derivative)] | ||
| #[derivative(Clone(bound = ""))] | ||
| pub struct GridViewTemplate<E: 'static, M: frp::node::Data, P: frp::node::Data> { |
There was a problem hiding this comment.
As requested earlier, please use better names than "E", "M", and "P" ora add docs.
| pub struct Model<E, P> { | ||
| display_object: display::object::Instance, | ||
| visible_entries: RefCell<HashMap<(Row, Col), E>>, | ||
| free_entries: RefCell<Vec<E>>, | ||
| entry_creation_ctx: EntryCreationCtx<P>, | ||
| } |
There was a problem hiding this comment.
You are using <E, P> here instead of <E::Entry>, right?
- Thank you so much for caring about it.
- I think that this might be a corner case where we could actually allow
<E::Entry>. Let's have a short call about it tmrw :)
There was a problem hiding this comment.
After a call, we decided to keep <Entry, EntryParams> instead of <E: Entry>.
| @@ -0,0 +1,123 @@ | |||
| //! A module with [`VisibleArea`] structure. | |||
There was a problem hiding this comment.
a short info of what it is would be appreciated :)
There was a problem hiding this comment.
Actually, it's an outdated doc.
| use crate::Viewport; | ||
|
|
||
|
|
||
|
|
| @@ -0,0 +1,92 @@ | |||
| //! A debug scene which shows the Select Component. The chosen entries are logged in console. | |||
There was a problem hiding this comment.
The chosen entries are logged in console.
I believe this is not true
…-view-scrolling-182585742
farmaazon
added
CI: Ready to merge
**Note**: This PR also contains content of previous Grid View PR. We decided to discard the previous, because this one did some refactoring of old one, and it's not a big addition. Added a scrollable::GridView component, which just embeds the GridView in ScrollArea. Also, re-worked the idea of text layers. https://user-images.githubusercontent.com/3919101/179020359-512ee127-c333-4f86-bff5-f1cb4154e03c.mp4
Pull Request Description
Note: This PR also contains content of previous Grid View PR. We decided to discard the previous, because this one did some refactoring of old one, and it's not a big addition.
Added a scrollable::GridView component, which just embeds the GridView in ScrollArea. Also, re-worked the idea of text layers.
grid_view-.3.mp4
Important Notes
Checklist
Please include the following checklist in your PR:
Scala,
Java,
and
Rust
style guides.
./run ide distand./run ide watch.