-
Notifications
You must be signed in to change notification settings - Fork 29
Save and restore the dock's display state from persistent storage #422
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
base: main
Are you sure you want to change the base?
Conversation
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.
a few nits about naming clarity:
- The word "prompt" should be something like "notice" instead, since a prompt implies that we're asking the user to input some info. But really, we're just showing the user a notice that something is happening in the background.
- Similarly, the word "timeout" should be replaced with "failure"/"failed"/"error", since timing out is just one potential error that could occur when loading a room.
See my other comments about how to potentially redesign the action handling logic for Pending, Success, and Timeout.
@alanpoon is this blocked on makepad/makepad#711? |
Yes |
src/app.rs
Outdated
/// it should show the room's timeline and hide its `Pending` message. | ||
/// 2. When changing from the desktop to mobile view, we want to ensure that | ||
/// all newly-restored rooms that were loaded in the background are properly displayed. | ||
pub all_known_rooms_loaded: bool, |
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.
use cached widget rooms_list
dad1373
to
fc29024
Compare
4a19157
to
88252db
Compare
Waiting for this PR to enhancement to Makepad makepad/makepad#726 |
8e19eeb
to
35ab460
Compare
0e23051
to
480813f
Compare
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.
Pull Request Overview
This PR introduces the functionality to persist and restore the dock's and window's layout state in the application. The changes save the layout when the UI view is hidden and upon app shutdown and then restore these states on startup. Key changes include:
- Adding functions to load and restore the rooms panel and window geometry state.
- Enhancing UI components (RoomScreen, MainDesktopUI, etc.) to handle state restoration.
- Modifying persistent state management by introducing new serialization routines for the restored types.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/sliding_sync.rs | Added asynchronous functions to load and restore panel/window state. |
src/persistent_state.rs | Introduced save/load functions for dock and window state. |
src/home/rooms_list.rs | Added helper methods to check room loading status. |
src/home/room_screen.rs | Updated logic to check room state and handle restoration actions. |
src/home/main_desktop_ui.rs | Updated UI logic to trigger dock state restoration on first draw. |
src/app.rs | Integrated persistent state saving on shutdown and state restoration actions. |
Comments suppressed due to low confidence (1)
src/home/main_desktop_ui.rs:373
- Consider revising this error message to provide clearer context about the failure and removing the 'BUG' prefix, as it may be confusing in production logs.
error!("BUG: failed to load dock state upon DockLoad action.");
// Obtain the current user's power levels for this room. | ||
submit_async_request(MatrixRequest::GetRoomPowerLevels { room_id: room_id.clone() }); | ||
|
||
let state_opt = TIMELINE_STATES.lock().unwrap().remove(&room_id); | ||
let (mut tl_state, first_time_showing_room) = if let Some(existing) = state_opt { | ||
(existing, false) | ||
} else { | ||
let (update_sender, update_receiver, request_sender) = take_timeline_endpoints(&room_id) | ||
.expect("BUG: couldn't get timeline state for first-viewed room."); | ||
let Some((update_sender, update_receiver, request_sender)) = take_timeline_endpoints(&room_id) else { |
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.
[nitpick] Consider logging a warning when timeline endpoints are not available instead of silently returning, to help diagnose issues during the first view of a room.
let Some((update_sender, update_receiver, request_sender)) = take_timeline_endpoints(&room_id) else { | |
let Some((update_sender, update_receiver, request_sender)) = take_timeline_endpoints(&room_id) else { | |
warn!("Timeline endpoints are not available for room_id: {}", room_id); |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Fixes #414
Screen.Recording.2025-03-12.at.1.21.32.PM.mov
If the user has resized a dock pane by dragging the splitter, that is currently not preserved.
This can also be offered as a preference/setting in the settings pane, once that is implement.
I have also explored saving and restoring the window dimension. Currently makepad does not have a solution resize the window dynamically.