Skip to content

feat: Track profile modifications with *#26

Open
Eclipse-Dominator wants to merge 5 commits intoToRvaLDz:mainfrom
Eclipse-Dominator:issue-24-branch
Open

feat: Track profile modifications with *#26
Eclipse-Dominator wants to merge 5 commits intoToRvaLDz:mainfrom
Eclipse-Dominator:issue-24-branch

Conversation

@Eclipse-Dominator
Copy link
Copy Markdown
Contributor

@Eclipse-Dominator Eclipse-Dominator commented Apr 20, 2026

Closes #25

Summary

Refactors profile management in Monique to better track unsaved modifications and improves the dialog system with callback-based flows.

Whenever the user modifies anything, if _dirty is not true, then Monique will now snapshot the current profile environment and named it as "XXX" + "*". e.g. ("Profile A" => "Profile A*") and append it to the relevant location in dropdown and switches to it. This also applies to "(Current)" => "(Current)*". (latest commit)

When the user reverts, the old monitor.conf is reapplied but now the settings will not switch to the current reported one by IPC but keeping whatever the user was working on before. (first commit)

When the user tries to switch to a different profile with unsaved changes, it now prompts the user to save/discard/cancel their current modified profile. Should they save or discard, the temporary snapshot will be discarded.

As this PR contains multiple commits which includes refactors that can be more generalized, I suggest rebasing the commits instead of merging all the commits together if it is approved

Changes and relevant bug fixes

1. fix: Keep modifications instead of overwriting from IPC during revert

  • Simplify revert logic - don't reload monitors from IPC, keep user's current GUI state
  • Removes pre_apply_monitors tracking
  • When user reverts, just reload compositor config instead of overwriting GUI state from IPC
  • Resolves revert consistency between workspace and display config

2. refactor: Add callbacks to save dialog

  • Add success_cb and cancel_cb parameters to _on_save_clicked()
  • Pass callbacks through to _on_save_response()
  • Call callbacks at appropriate times after save completes
  • Add _save_then_switch_saved_profile() that uses success callback (this replaces the old _on_save_clicked()
  • Main purpose is to facilitate future changes that will benefit through the implementation of callback

3. refactor: Extract _load_profile method

  • Extract profile loading logic into dedicated _load_profile(Profile) method
  • Reduces code duplication in profile selection
  • Main purpose is to facilitate future changes that will benefit through this
  • Side effects: fixes a bug where upon initial load, a named profile is selected but the values are incorrectly loaded

4. refactor: Replace close dialog with callback-based unsaved changes check

  • Add reusable _check_unsaved_changes(body_text, on_save, on_cancel, on_discard) function
  • Uses callback pattern to enable more customizable function call
  • Main purpose is to facilitate future changes that will benefit through the implemementation of callback
  • Side effect: when user exits with unsaved changes, the program will now exit after the user saves successfully, before, the exit will be aborted and the GUI will switch to the newly saved profile

5. feat: Track modifications with * profile entry

  • Create modified profile entry in dropdown with "*" suffix when user edits settings
  • Better UX - modified profiles shown as separate entry, not switching to "(Current)"
  • Update _on_profile_selected() to handle dirty state with save/discard/cancel prompts using callbacks
  • Update _load_current_state() to reset profile name when loading current state
  • When deleting profile, _toast now reads "Discarding changes" instead if the current profile is unsaved

Overview of workflow modifed by this PR

Summary of Modified Flows

  1. Save Profile Flow
  • _on_save_clicked(success_cb, cancel_cb)_on_save_response() → calls optional callbacks after save completes or fails
  • Added _save_then_switch_saved_profile() that uses success_cb to select the saved profile (this imitates the old _on_saved_clicked behavior and is used to replace GUI save and ctrl+S

  1. Dirty Tracking Flow
_mark_dirty()
├── Creates _current_modified_profile (name="SelectedName*")
├── _refresh_profile_list() → inserts "*" profile in dropdown
└── _select_profile_by_name() → selects it

Modified profile stores all current monitor and workspace config / snapshot current version

  1. Profile Selection Flow
_on_profile_selected()
├── if dirty? 
│   ├── Yes → _on_switch_profile_request() → prompt save/discard/cancel
│   │         ├── save → _on_save_clicked() → remove_modified_profile() → switch_to_selected()
│   │         ├── discard → remove_modified_profile() → switch_to_selected()
│   │         └── cancel → revert to last profile
│   └── No → switch_to_selected()
│
└── switch_to_selected()
    ├── if (Current) → _load_current_state()
    └── if named profile → _load_profile()

  1. Close/Switch Dialog Flow
    Refactored general check unsaved change dialog function
_check_unsaved_changes(body_text, on_save, on_cancel, on_discard)
├── if not dirty → return False
└── shows dialog → on_response() calls appropriate callback
    ├── cancel → on_cancel()
    ├── save → on_save()
    └── discard → on_discard()

Using the above, these 2 additional functions are created one to handle when user closes Monique
and the other to handle when user switches profile when they have unsaved changes

_on_close_request() → _check_unsaved_changes(on_save=save_and_close, on_discard=close)
_on_switch_profile_request() → wrapper for profile switching

  1. Profile Loading
    Refactored to reduce code duplication and maintain consistency
_load_profile(profile)  ← extracted method
├── self._monitors = profile.monitors
├── self._workspace_rules = profile.workspace_rules  
├── self._current_profile_name = profile.name
├── self._canvas.monitors = ...
├── _update_properties_for_selected()
└── _update_clamshell_indicators()

Called by:

  • _select_matching_profile(), this fixes the issue where named profile is not properly loaded on first launch of monique
  • switch_to_selected() in _on_profile_selected()

@Eclipse-Dominator Eclipse-Dominator force-pushed the issue-24-branch branch 2 times, most recently from 427ef2e to 54f7925 Compare April 20, 2026 05:34
@ToRvaLDz
Copy link
Copy Markdown
Owner

Thanks for this! The direction is solid. The XXX* entry is much clearer than silently switching to (Current), and the callback refactor is the right move. A few things to address before merging.

🔴 Blockers

1. Type hints use lowercase callable

Several signatures use callable[[Profile], None] or callable | None:

def _on_save_clicked(self, btn, success_cb: callable[[Profile], None] | None = None, ...)
def _check_unsaved_changes(..., on_save: callable | None = None, ...)

callable is the builtin function. The proper type is Callable from typing:

from typing import Callable
success_cb: Callable[[Profile], None] | None = None

With from __future__ import annotations it won't crash at runtime, but it's incorrect and will fail type checking.

2. _on_save_response leaves inconsistent state on save failure

self._dirty = False
self._profile_mgr.save(profile)       # if this raises...
self._toast(f"Profile '{name}' saved")
if success_cb:
    success_cb(profile)

If save() raises, _dirty is already False, no toast, no callback, and cancel_cb never fires either. The UI state no longer matches the on-disk state. _dirty = False should happen after the save succeeds, ideally wrapped in try/except that calls cancel_cb on failure.

🟡 Medium

3. Stale comment in _on_apply_clicked

# Snapshot actual compositor state before applying (for revert)
self._ws_snapshot = self._ipc.get_workspaces()

The monitor snapshot was removed (good), but the comment still advertises a full compositor-state snapshot. Worth trimming to "Snapshot workspaces (for revert)".

4. Revert UX regression when not dirty

New _do_revert:

self._toast("Reverted to last configuration")

If a user clicks Apply, doesn't modify anything, then clicks Revert, the monitor.conf file is rolled back but the GUI still shows the just-applied layout. From their perspective, nothing visibly changed. Consider either:

  • Calling _load_current_state() as a fallback when not self._dirty
  • A clearer toast explaining the file was reverted but GUI state preserved

🟢 Minor

  • Typos in commit messages and inline comments: implemementation, wtv, diff
  • _current_modified_profile isn't cleared after a successful save (it'll be overwritten on the next _mark_dirty, so no functional bug, but dangling state)
  • _check_unsaved_changes returns bool that only indicates "dialog was shown". The docstring could clarify that.

Merge strategy

Agreed on rebase-merge to preserve the 5 logical commits. Much better for git bisect later.

@Eclipse-Dominator Eclipse-Dominator force-pushed the issue-24-branch branch 2 times, most recently from f42df4c to d8f7b15 Compare April 20, 2026 19:55
ZQL added 5 commits April 21, 2026 04:01
When reverting configuration, only restore the monitor.conf file and
reload the compositor. Keep current GUI values in the UI instead
of restoring from pre-apply snapshots to preserve user edits.
- Add success_cb and cancel_cb parameters to `_on_save_clicked`
- Pass callbacks through `_on_save_response`
- Call callbacks at appropriate times after save completes
- Add `_save_then_switch_saved_profile` that replaces the old
`_on_saved_clicked` functionality
- Extract profile loading logic into _load_profile()
- Update _select_matching_profile to use _load_profile
- Add _save_then_switch_saved_profile using success callback
- Update save button and Ctrl+S to use new method
- Add _check_unsaved_changes with callback parameters
- Update _on_close_request to use callbacks
- Remove old _on_close_dialog_response
- Add _current_modified_profile field
- Update _refresh_profile_list to include modified profile when dirty
- Update _mark_dirty to create modified profile with * suffix
- Add _on_profile_selected to prompt save/discard/cancel using callbacks
- Update _load_current_state to reset _current_profile_name
@Eclipse-Dominator
Copy link
Copy Markdown
Contributor Author

Eclipse-Dominator commented Apr 20, 2026

overall diff from last change: https://github.com/ToRvaLDz/monique/compare/54f79259a5d4a402dca6f3714e0aaea634ad1b86..0e804f3a19accb2d76243df6d4cfc46ad13196c3

Hi, thanks for the review, I have made the relevant changes as requested to each of the 5 commits with exception of some of the suggestions in 4th point:

For 4,

  1. Revert UX regression when not dirty
    If a user clicks Apply, doesn't modify anything, then clicks Revert, the monitor.conf file is rolled back but the GUI still shows the just-applied layout. From their perspective, nothing visibly changed. Consider either:
    Calling _load_current_state() as a fallback when not self._dirty
    A clearer toast explaining the file was reverted but GUI state preserved

I updated the toast to self._toast("Configuration restored from backup") and updated the relevant comments.
As for whether to fallback when not self._dirty, I think it is a bit unnatural from my standpoint as whatever the user is reverting to might not be edited at all.
e.g.
user applied 'Profile A' => switches to 'Profile B' and applies it => didnt like the result => revert.
Throughout this process, the user never made any edits so self._dirty is always false but the we still should not revert to _load_current_state() since it will miss out some values from loading from IPC.

Then for the case where nothing happens during apply, clicking on revert should not 'change' anything since that was the state before previously.

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.

[Feature] Better modification and revert behaviors

2 participants