Extract parts of gui into smaller components #7
Conversation
src/gui.rs
Outdated
| Some("https://raw.githubusercontent.com/tudsat-rocket/sam/main/archive/euroc_2023_flash_filtered.json"), | ||
| ), | ||
| ]; | ||
| use crate::gui::windows::archive::open_archive_window; |
There was a problem hiding this comment.
Definitely a fan of moving this somewhere else. In the future, I'd like to have some sort of Archive struct that is a bit smarter about things like caching, but that would probably be independent of the Window. Also like the structure with a separate windows folder. We might have more of these in the future.
There was a problem hiding this comment.
I totally agree, it's more predictable and more similar to how big UI are built
There was a problem hiding this comment.
Overall I think this looks good, and it's definitely overdue to move some of this stuff out of gui.rs. I have some concerns regarding passing Sam around, but I think those don't necessarily have to be addressed completely right now. The only other suggestion I would make is maybe introducing a new folder src/gui/panels, as I think panels are sufficiently different (get passed Context instead of Ui for instance) from other UI widgets in egui (similar to windows) to warrant it.
src/gui.rs
Outdated
| }); | ||
| }); | ||
| }); | ||
| create_top_menu_bar(self, ctx); |
There was a problem hiding this comment.
I have to say I'm not the biggest fan of passing self around to other files because that might make it more difficult in the future to tell where the contents of Sam are being modified. On the other hand I can see why it would be easier this way.
I think we could pass in something for set_enabled (and this would probably be done similarly for the other panels), and a reference to the data source, but for everything where we change state from within the panel, we might have to implement a response system similar to other egui widgets. So for instance having the function/method return some sort of enum like:
enum TopBarResponse {
ArchiveWindowOpened,
TabSelected(GuiTab),
// etc
}That's a bit more involved, so I think it may not be necessary to do that right now, but I think long-term this would be the cleanest solution, even though It would mean a bit more code in src/gui.rs.
src/gui.rs
Outdated
| }); | ||
| }); | ||
| ui.add_space(10.0); | ||
| open_archive_window(ctx, &mut archive_open, self); |
There was a problem hiding this comment.
Similar to above, great fan of moving some of the more specific UI stuff out of here though. For windows it might be interesting to have a system where windows can return values, in this case a flight log, to src/gui.rs. Something along the lines of:
if let Some(log) = ui.add(archive_window).result() {
self.open_archive_log(log);
}Same as above though, this isn't necessarily urgent.
src/gui.rs
Outdated
| } | ||
| }); | ||
| }); | ||
| create_simulation_panel(self, ctx); |
There was a problem hiding this comment.
This might be a place where it would be possible to get rid of passing self, by just passing a boolean for enabling and the data source. I also was never a huge fan of how we check for a simulation data source here. Ideally we should try downcasting the boxed trait inside self.data_source to the actual SimulationDataSource type, and then passing that. But again, that's not necessarily relevant to the cleanup.
src/gui.rs
Outdated
| }); | ||
| }); | ||
| } | ||
| create_header(self, ctx); |
There was a problem hiding this comment.
This looks good except for the same concerns about self. I would probably also use the opportunity to move self.top_bar out of here as well.
src/gui.rs
Outdated
| }); | ||
| }); | ||
| }); | ||
| create_bottom_status_bar(self, ctx); |
There was a problem hiding this comment.
I was planning to either get rid of this reset button altogether or move it to the data source specific UI, for resetting during long serial sessions. This way, there would be no actual GUI code here, just some generic GUI for the data source and current tab. In that case, I think this could be compact enough to stay here. This way all the tab-dependent stuff would happen here in src/gui.rs (except maybe for the buttons for selecting one, but that could be changed) and if you added a new tab you would only have to add your new tab to src/gui.rs.
src/gui.rs
Outdated
| } | ||
| }); | ||
| // Everything else. This has to be called after all the other panels are created. | ||
| create_body(self, ctx); |
There was a problem hiding this comment.
I think this might be general enough to stay in here. See reasoning above regarding keeping all the match self.tab { ... } stuff in one place.
While doing that, clean up those two a bit, move Reset button to serial data source UI. Also, rename components to panels.
Also, try to avoid passing self in most places.
KoffeinFlummi
deleted the
refactor/extract-parts-of-gui-into-smaller-components
branch
No description provided.