Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Terminal]: 'Save' and 'Discard Changes' button should remain in disabled state until we don't make any changes inside Settings page. #12424

Open
ghost opened this issue Feb 8, 2022 · 2 comments
Open

[Terminal]: 'Save' and 'Discard Changes' button should remain in disabled state until we don't make any changes inside Settings page. #12424

ghost opened this issue Feb 8, 2022 · 2 comments
Labels
A11yCO Accessibility tracking A11yUsable Accessibility tracking Area-Accessibility Issues related to accessibility Area-SettingsUI Anything specific to the SUI HCL-E+D Accessibility tracking HCL-WindowsTerminal Accessibility tracking Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Milestone
Backlog

Comments

@ghost
Copy link

ghost commented Feb 8, 2022

Windows Terminal version

1.13.10336.0

Windows build number

10.0.22543.1000

Other Software

Test Environment:
OS: Windows 11 Version Dev (OS Build 22543.1000)
App: Windows Terminal Preview
Screen Reader: Narrator

Steps to reproduce

Repro Steps:

  1. Open Windows Terminal.
  2. Open Settings page using 'Ctr+,'.
  3. Observe the issue.

User Experience:
Users UX will not be good especially Screen Reader as nothing will happen when they activate 'Save' and 'Discard' as result they will not be able to figure out whether these buttons are actionable or not.

Attachments:
'Save' and 'Discard Changes' button should remain in disabled state until we don't make any changes..zip

Expected Behavior

'Save' and 'Discard Changes' button should remain in disabled state until we don't make any changes inside Setting page.

Actual Behavior

'Save' and 'Discard Changes' buttons are in active state even if they are not actionable inside Setting page.
@ghost ghost added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Accessibility Issues related to accessibility HCL-E+D Accessibility tracking A11yUsable Accessibility tracking HCL-WindowsTerminal Accessibility tracking A11yCO Accessibility tracking Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Feb 8, 2022
@ghost ghost changed the title [Terminal]: 'Save' and 'Discard Changes' button should remain in disabled state until we don't make any changes. [Terminal]: 'Save' and 'Discard Changes' button should remain in disabled state until we don't make any changes inside Settings page. Feb 8, 2022
@ghost ghost mentioned this issue Feb 8, 2022
Closed
@zadjii-msft zadjii-msft added Area-SettingsUI Anything specific to the SUI Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-3 A description (P3) and removed Issue-Bug It either shouldn't be doing this or needs an investigation. labels Feb 8, 2022
@zadjii-msft zadjii-msft added this to the Backlog milestone Feb 8, 2022
@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Feb 8, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 8, 2022
@zadjii-msft zadjii-msft mentioned this issue Feb 8, 2022
Open
25 tasks
@carlos-zamora
Copy link
Member

This depends on #9207. Once that's complete, we can detect if changes have been made, and enable/disable the buttons.

We probably need to use SettingContainer to detect when a change occurred, and update some global view model property s_ChangesDetected = true.

Come to think of it, it would be cleaner to do this with MVVM done, but we could just store that static property off of MainPage (or MainPage's new view model, which we then populate more later). That would be a step in the right direction.

@akash07k
Copy link

@carlos-zamora Absolutely.
MVVM pattern in settings UI will be a lot appreciated.
It will make the things more flexible

@carlos-zamora carlos-zamora mentioned this issue Jun 27, 2022
Open
65 tasks
@carlos-zamora carlos-zamora mentioned this issue Aug 7, 2024
Merged
DHowett pushed a commit that referenced this issue Aug 23, 2024
@carlos-zamora
Track and log changes to settings (#17678)
0a9cbd0 
Adds functionality throughout the settings model to keep track of which
settings have been set.

There are two entry points:
- AppLogic.cpp: this is where we perform a settings reload by loading
the JSON
- MainPage.cpp: this is where the Save button is clicked in the settings
UI

Both of these entry points call into
`CascadiaSettings::LogSettingChanges()` where we aggregate the list of
changes (specifically, _which_ settings changed, not _what_ their value
is).

Just about all of the settings model objects now have a
`LogSettingChanges(std::set& changes, std::string_view context)` on
them.
- `changes` is where we aggregate all of the changes to. In it being a
set, we don't need to worry about duplicates and can do things like
iterate across all of the profiles.
- `context` prepends a string to the setting. This'll allow us to better
identify where a setting was changes (i.e. "global.X" are global
settings). We also use this to distinguish between settings set in the
~base layer~ profile defaults vs individual profiles.

The change log in each object is modified via two ways:
- `LayerJson()` changes: this is useful for detecting JSON changes! All
we're doing is checking if the setting has a value (due to inheritance,
just about everything is an optional here!). If the value is set, we add
the json key to the change log
- `INHERITABLE_SETTING_WITH_LOGGING` in IInheritable.h: we already use
this macro to define getters and setters. This new macro updates the
setter to check if the value was set to something different. If so, log
it!

 Other notes:
- We're not distinguishing between `defaultAppearance` and
`unfocusedAppearance`
- We are distinguishing between `profileDefaults` and `profile` (any
other profile)
- New Tab Menu Customization:
- we really just care about the entry types. Handled in
`GlobalAppSettings`
- Font:
- We still have support for legacy values here. We still want to track
them, but just use the modern keys.
- `Theme`:
- We don't do inheritance here, so we have to approach it differently.
During the JSON load, we log each setting. However, we don't have
`LayerJson`! So instead, do the work in `CascadiaSettings` and store the
changes there. Note that we don't track any changes made via setters.
This is fine for now since themes aren't even in the settings UI, so we
wouldn't get much use out of it anyways.
- Actions:
- Actions are weird because we can have nested and iterable actions too,
but `ActionsAndArgs` as a whole add a ton of functionality. I handled it
over in `Command::LogSettingChanges` and we generally just serialize it
to JSON to get the keys. It's a lot easier than dealing with the object
model.

Epic: #10000
Auto-Save (ish): #12424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A11yCO Accessibility tracking A11yUsable Accessibility tracking Area-Accessibility Issues related to accessibility Area-SettingsUI Anything specific to the SUI HCL-E+D Accessibility tracking HCL-WindowsTerminal Accessibility tracking Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@akash07k @carlos-zamora @zadjii-msft