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.

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

Implement alert for unsaved changes #343

Merged
merged 5 commits into from
Jul 21, 2024

Conversation

jackyzy823
Copy link
Contributor

@jackyzy823 jackyzy823 commented Feb 25, 2024

For #296

There's two model: 1) Tracking if changed (not very accurately ie: Enable and disable and then enable is still treated as changed) 2) Comparison between current and original

It looks like SmartProxy has implemented some basic objection for tracking (settingsPage.changeTracking). So I use the tracking model for most changes.

However , it's hard/(too complex) to apply tacking model on GeneralOptions , But comparison is suitable here.


Tracking Action Element Status
activeProxy Change Profile cmbActiveProxyServer DONE
smartProfiles Edit profilename txtSmartProfileName DONE
smartProfiles Toggle Enable/Disable chkSmartProfileEnabled DONE
smartProfiles Choose Profile's Proxy Server cmbProfileProxyServer DONE
smartProfiles Add/Edit rule btnSubmitRule DONE
smartProfiles Add multiple rules btnSubmitMultipleRule DONE
smartProfiles Import rule btnImportRules DONE
smartProfiles Remove rule btnRulesRemove DONE
smartProfiles Clear all rules btnClearProxyRules DONE
smartProfiles Delete multiple rules btnRemoveMultipleProxyRule DONE in #341
rulesSubscriptions Subscribe/Edit a rules list btnSaveRulesSubscriptions Already
rulesSubscriptions Delete rules list btnRuleSubscriptionsRemove Already
rulesSubscriptions Clear all rules lists btnClearRulesSubscriptions Already
rulesSubscriptions Refresh rules list btnRuleSubscriptionsRefresh Already
rulesSubscriptions Delete multiple rules btnRemoveMultipleRulesSubscription DONE in #341
servers Add/Edit server btnSubmitProxyServer Already
servers Delete server btnServersRemove Already
servers Clear all servers btnClearProxyServers Already
servers Import server btnImportProxyServer DONE
servers Delete multiple servers btnRemoveMultipleProxyServer DONE in #341
serverSubscriptions Subscribe list btnSaveServerSubscription Already
serverSubscriptions Delete list btnSubscriptionsRemove Already
serverSubscriptions Clear all lists btnClearServerSubscriptions Already
serverSubscriptions Delete multiple lists btnRemoveMultipleServerSubscription DONE in #341
SPECIAL Import server backup btnImportProxyServer SAME as Import server
SPECIAL Restore backup btnRestoreBackup IGNORE unsaved changes by removing eventListenr
SPECIAL Factory Reset btnFactoryReset IGNORE unsaved changes by removing eventListenr
SPECIAL Delete a profile btnDeleteSmartProfile NOTHING (because runtime message is send directly)
SPECIAL Add a new profile if user choose to save when staying , try save it (intent to fail if profile title is empty)

Also changeTracking to false when runtimeSendMessage(SettingsPageSave*) is called.

Note:
changeTracking .rulesSubscriptions should also set to false in onClickSaveSmartProfile ( same as changeTracking.smartProfile )

@salarcode
Copy link
Owner

Thanks for this, I will check it soon.
I wish if you were to work on a feature request you worked on something higher priority, like these:
#273 , #317 , #342 and etc
If you want to work on anything please let me know I'll refine the issue details

@jackyzy823
Copy link
Contributor Author

I may work on #273 , but implementation details should be discussed. (like export in what format or etc...)

I really don't understand what #317 is for.
I won't work on #342 , because i don't have any mac devices.

Currently i'm working on SwitchyOmega import related stuff like #318.

@salarcode
Copy link
Owner

salarcode commented Feb 26, 2024

Thanks. I have already started working on #318 , #286 so that one is not needed. Lots of requests for it recently

@jackyzy823
Copy link
Contributor Author

Well, we could discuss some implementation details.
I'll add some comments under #286

src/core/definitions.ts Show resolved Hide resolved
src/ui/code/settingsPage.ts Outdated Show resolved Hide resolved
src/ui/code/settingsPage.ts Show resolved Hide resolved
@salarcode
Copy link
Owner

salarcode commented Mar 7, 2024

Thanks for the write up, overall looking good but I've left some comments.

I've pushed a clean up commit as well. Please fix the conflicts while you are at it

…pr/343

# Conflicts:
#	src/_locales/default-messages.json
#	src/ui/code/settingsPage.ts
@salarcode salarcode merged commit 1bb8c7f into salarcode:master Jul 21, 2024
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.

None yet

2 participants