chatterino2 icon indicating copy to clipboard operation
chatterino2 copied to clipboard

Newly added highlights aren't disregarded when "cancelling" settings window

Open jupjohn opened this issue 3 years ago • 5 comments

Checklist

  • [X] I'm reporting a problem with Chatterino
  • [X] I've verified that I'm running the most recent nightly build or stable release
  • [X] I've looked for my problem on the wiki
  • [X] I've searched the issues and pull requests for similar looking reports

Describe your issue

When pressing "cancel" on the settings window when adding a new highlight, the newly added highlight is saved instead of being disregarded.

Screenshots

https://user-images.githubusercontent.com/4962764/152666157-b4e9b871-5479-49a7-82fa-d10d23f53c80.mp4

OS and Chatterino Version

Chatterino 2.3.4 (commit 5490ff50) on Arch Linux (5.16.5), bspwm

jupjohn avatar Feb 06 '22 03:02 jupjohn

This is true with basically everything, Commands, Filters, Highlights, Ignores, Nicknames - our cancel button is basically cosmetic for everything that isn't a setting.

Felanbird avatar Feb 06 '22 03:02 Felanbird

@Felanbird want me to expand this issue's scope to cover all of those (I'll dig into them for a complete list) or leave as is?

jupjohn avatar Feb 06 '22 11:02 jupjohn

@Felanbird want me to expand this issue's scope to cover all of those (I'll dig into them for a complete list) or leave as is?

yeah sure if u want

Felanbird avatar Feb 06 '22 11:02 Felanbird

src/singletons/Settings.cpp constructor calls persist on the SignalVectors, which loads in the current setting value, and ensures that any time they are modified that the underlying setting is set, however, there's no mechanism saying "when the setting is modified from other means, revert it back to this value" which is how all other settings work with the reverting.

I see two options:

  1. In the persist function, add some check back, so that if something under the /highlighting/badges setting has been modified, feed it back to the SignalVector. I'm not 100% sure if this is feasible with the settings library, but it should probably be the first option we test.
  2. Add a mechanism to SignalVectors for saving/recalling states and similar to how we do it for other settings, so in SettingsDialog.cpp we'd not only save snapshot of the settings, but also save a snapshot for each SignalVector used for settings

pajlada avatar Feb 06 '22 14:02 pajlada

2. Add a mechanism to SignalVectors for saving/recalling states and similar to how we do it for other settings, so in SettingsDialog.cpp we'd not only save snapshot of the settings, but also save a snapshot for each SignalVector used for settings

This sounds like the correct approach to me. Keeps the functionality the same as the rest of the settings. I'm not too familiar with the SignalVector class, can't comment on the technical aspect of that.

jupjohn avatar Feb 06 '22 22:02 jupjohn