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

gui: Implement close confirmation. #2216

Merged
merged 4 commits into from
Jul 13, 2021
Merged

gui: Implement close confirmation. #2216

merged 4 commits into from
Jul 13, 2021

Conversation

denravonska
Copy link
Member

This adds an optional confirmation on close to avoid accidentally closing the main wallet window. If the user also has enabled minimize on close that will take priority and the confirmation dialog will not show up until the wallet is closed from the system tray.

The existing quit handling was slightly alter to more closely resemble that of the current Bitcoin code base.

Tested on Linux:

  • X from main window with option enabled/disabled
  • X from main window with confirm and minimize enabled
  • X from systray with confirm and minimize enabled
  • kill process with confirm enabled (no confirmation dialog shown)

Needs testing:

  • Windows
  • OSX

This closes #2211

@denravonska
Copy link
Member Author

Another thing that might be worth adding to this PR is the check for a systray availability. If non exists on the OS then Bitcoin hides the options that rely on it.

@jamescowens jamescowens changed the title Implement close confirmation. gui: Implement close confirmation. Jul 10, 2021
@jamescowens jamescowens added this to the Janice milestone Jul 10, 2021
@jamescowens jamescowens self-requested a review July 10, 2021 13:47
denravonska
denravonska commented Jul 10, 2021
View reviewed changes
src/qt/bitcoingui.cpp Outdated Show resolved Hide resolved
@jamescowens
Copy link
Member

I will test this on Windows later on today.

@jamescowens
Copy link
Member

jamescowens commented Jul 11, 2021 •
edited
Loading

Behavior with the wallet syncing from a few thousand blocks back, so that it is working hard, with the new setting enabled and logging out...
image

Behavior with setting enabled and disabled identical. Takes a few seconds to exit but eventually exits safely.

@jamescowens
Copy link
Member

Also checked everything you did. The behavior is as expected.

I don't have a Mac, so I can't do testing there.

@denravonska
Copy link
Member Author

Mac should be a no-op. I'll see if I can fire up the old MacBook to do some testing.

@jamescowens jamescowens modified the milestones: Janice, Ingrid Jul 11, 2021
@ghost
Copy link

ghost commented Jul 12, 2021

Normal behaviour when exiting an application on MacOS by the red X is that it minimise and run in the dock.

Can't test due to the setting resetting when exiting the setting window.

@jamescowens
Copy link
Member

Should we suppress this setting entirely on the Mac?

@denravonska
Copy link
Member Author

@jamescowens They are suppressed now in the sense that the signals are ignored. However, upstream Bitcoin has solved it more elegantly (mostly) where they check for the existence of a systray instead of basing it purely on the target platform.

@jamescowens
Copy link
Member

@sweede-se what setting exactly is resetting when you exit the setting window?

@ghost
Copy link

ghost commented Jul 12, 2021

@jamescowens Minimize to tray, Minimize on close and Confirm on close. They all get unselected. If they aren't supposed to work on MacOS it would be better if they wasn't shown.

@jamescowens
Copy link
Member

@denravonska can we do a conditional hide of the checkboxes for these on the Mac?

@denravonska
Copy link
Member Author

@denravonska can we do a conditional hide of the checkboxes for these on the Mac?

I'll have a look. Come to think of it, the confirm on close should be enabled on OSX.

@denravonska
Copy link
Member Author

@sweede-se Can you try it now? The options are still there but should be disabled on macOS. I kept them visible since that's how Bitcoin does it.

@ghost
Copy link

ghost commented Jul 13, 2021

@denravonska will test it tonight when I get home from work.

@denravonska denravonska force-pushed the feature/2211-confirm-on-close branch from 42dff8b to e125602 Compare July 13, 2021 07:46
@denravonska
Implement close confirmation.
f3ad5cd 
This adds an optional confirmation on close to avoid accidentally closing the
main wallet window. If the user also has enabled minimize on close that will
take priority and the confirmation dialog will not show up until the wallet
is closed from the system tray.

The existing quit handling was slightly alter to more closely resemble that
of the current Bitcoin code base.

Issue: gridcoin-community#2211
@denravonska
Make sure we have both client- and options model before using them.
d2f43c8
@denravonska
Bring options behavior more in line with upstream.
7b3e462 
- Remove startup options on OSX
- Only map systray related options if ther is a systray

Additionally, the Window tab is now enabled on OSX since Gridcoin have added
additional Window options which are relevant for OSX.
@denravonska denravonska force-pushed the feature/2211-confirm-on-close branch from e125602 to 047566b Compare July 13, 2021 10:54
@denravonska denravonska force-pushed the feature/2211-confirm-on-close branch from 047566b to 2ceac62 Compare July 13, 2021 11:21
@jamescowens jamescowens merged commit f47664e into gridcoin-community:development Jul 13, 2021
@ghost
Copy link

ghost commented Jul 13, 2021

Should I still check it out? Witch branch?

@jamescowens
Copy link
Member

It is merged into the testnet branch now as RC4. Yes it would be good to check it.

@denravonska
Copy link
Member Author

There might be some quirks on macOS but they should be small enough to be fleshed out during the RCs.

@ghost
Copy link

ghost commented Jul 14, 2021

Confirm on close is now not possible to mark.

jamescowens added a commit to jamescowens/Gridcoin-Research that referenced this pull request Aug 1, 2021
@jamescowens
5.3.2.0, 2021-08-01, leisure, "Ingrid"
fe6ab87 
Added
 - util, rpc. gui: Changes for snapshotdownload and add feature sync from zero gridcoin-community#2093 (@iFoggz)
 - gui: Implement GUI version of consolidateunspent (coin control part) gridcoin-community#2111 (@jamescowens)
 - gui: Implement consolidateunspent wizard gridcoin-community#2125 (@jamescowens)
 - qt: Add antialiasing to traffic graph widget gridcoin-community#2150 (@barton2526)
 - util: Port of ArgsManager and a significant subset of src/util gridcoin-community#2146 (@jamescowens)
 - doc: add issue templates for bug reports and feature requests gridcoin-community#2147 (@Pythonix)
 - gui, rpc: Implement dynamic stakesplitting control, settings changes via rpc, and dynamic changes to sidestaking via rpc gridcoin-community#2164 (@jamescowens)
 - rpc: Implement getblocksbatch gridcoin-community#2205 (@jamescowens)
 - voting, rpc, gui: Implement demand loading of historical poll by poll id and AVW calculation gridcoin-community#2210 (@jamescowens)
 - gui: Show GUI error dialog if command line parsing fails gridcoin-community#2218 (@jamescowens)
 - gui: Implement close confirmation. gridcoin-community#2216 (@denravonska)
 - build: Use -fstack-reuse=none gridcoin-community#2232 (@barton2526)

Changed
 - doc: Update build doc gridcoin-community#2078 (@iFoggz)
 - gui: Normalize button and input control appearance gridcoin-community#2096 (@cyrossignol)
 - consensus: Implement GetMinimumRequiredConnectionsForStaking gridcoin-community#2097 (@jamescowens)
 - refactor: move CTransaction to primitives gridcoin-community#2006 (@div72)
 - consensus, refactor, test: Merkle gridcoin-community#2094 (@div72)
 - gui: Update diagnostics gridcoin-community#2095 (@jamescowens)
 - gui: Refresh UI styles and sidebar/statusbar design gridcoin-community#2102 (@cyrossignol)
 - gui: Set standard base Qt style on Windows and macOS gridcoin-community#2114 (@cyrossignol)
 - build, refactor: bump to C++17 gridcoin-community#2113 (@div72)
 - util, rpc, gui: Implement GetMaxInputsForConsolidationTxn() gridcoin-community#2119 (@jamescowens)
 - gui: Refresh overview page design gridcoin-community#2117 (@cyrossignol)
 - depends: change boost mirror gridcoin-community#2122 (@div72)
 - refactor: small cleanup gridcoin-community#2123 (@div72)
 - build: Update depends Qt recipe to version 5.12.10 gridcoin-community#2129 (@cyrossignol)
 - build: Bump Codespell to 2.0.0 gridcoin-community#2135 (@barton2526)
 - gui: Refresh "send coins" page design gridcoin-community#2126 (@cyrossignol)
 - gui: Optimize locks to improve responsiveness gridcoin-community#2137 (@cyrossignol)
 - gui: Refresh "receive payment" page design gridcoin-community#2138 (@cyrossignol)
 - gui: Add empty placeholder to recent transactions list gridcoin-community#2140 (@cyrossignol)
 - gui: Refresh transaction history page design gridcoin-community#2143 (@cyrossignol)
 - gui: Refresh address book page design gridcoin-community#2145 (@cyrossignol)
 - doc: Update http to https where possible gridcoin-community#2148 (@barton2526)
 - depends: Update dependencies gridcoin-community#2153 (@barton2526)
 - depends: Bump python to 3.6 gridcoin-community#2159 (@barton2526)
 - test: Update cppcheck linter to c++17 gridcoin-community#2157 (@barton2526)
 - test: Drop Travis specific workarounds, Mention commit id in error, Fix typos, Update spellcheck ignore words gridcoin-community#2158 (@barton2526)
 - gui: Overhaul the voting UI gridcoin-community#2151 (@cyrossignol)
 - wallet: simplify nTimeSmart calculation gridcoin-community#2144 (@div72)
 - gui: Refresh checkbox and radio button styles gridcoin-community#2170 (@cyrossignol)
 - build: Bump libevent to 2.1.11 gridcoin-community#2172 (@barton2526)
 - build: Update native_mac_alias, Remove big sur patch file in qt recipe gridcoin-community#2173 (@barton2526)
 - docs: Misc Grammar gridcoin-community#2176 (@barton2526)
 - build: miniupnpc 2.2.2 gridcoin-community#2179 (@barton2526)
 - rpc: Refresh rainbymagnitude gridcoin-community#2163 (@jamescowens)
 - util: optimize HexStr gridcoin-community#2185 (@div72)
 - refactor: misc style changes gridcoin-community#2177 (@div72)
 - rpc: consolidatemsunspent changes. gridcoin-community#2136 (@iFoggz)
 - refactor: Replace "GlobalStatus" state management gridcoin-community#2183 (@cyrossignol)
 - rpc, util: Remove use of ArgsManager::NETWORK_ONLY for now gridcoin-community#2190 (@jamescowens)
 - doc: Replace hidden service with onion service, Capitalize "Tor" gridcoin-community#2193 (@barton2526)
 - gui: Update Qt Linguist localization files gridcoin-community#2192 (@cyrossignol)
 - script: Shell script cleanups gridcoin-community#2195 (@barton2526)
 - build: set minimum required Boost to 1.58.0 gridcoin-community#2194 (@barton2526)
 - build, util: Prevent execution for Windows versions less than Windows 7 gridcoin-community#2203 (@jamescowens)
 - build: Tweak NSIS Windows installer gridcoin-community#2204 (@jamescowens)
 - build: Add bison in depends gridcoin-community#2206 (@iFoggz)
 - build: macOS toolchain bump gridcoin-community#2207 (@div72)
 - doc: Update build-unix.md gridcoin-community#2212 (@springfielddatarecovery)
 - build: Bump minimum python version to 3.6, Remove python2 references gridcoin-community#2219 (@barton2526)
 - depends: Change openSSL source path to Github gridcoin-community#2237 (@barton2526)
 - doc: Fix typo in bug report template gridcoin-community#2243 (@jamescowens)
 - ci: fold depends output gridcoin-community#2244 (@div72)

Removed
 - wallet: remove dead hardcoded addnodes gridcoin-community#2116 (@sweede-se)
 - rpc: Remove readconfig gridcoin-community#2248 (@jamescowens)
 - rpc: Remove obsolete comparesnapshotaccrual RPC function gridcoin-community#2100 (@jamescowens)
 - rpc: Remove memorypool RPC Command gridcoin-community#2214 (@RoboticMind)
 - rpc: Remove deprecated RPC commands gridcoin-community#2101 (@jamescowens)
 - Remove CCT from README, add Discord gridcoin-community#2134 (@barton2526)
 - refactor: Remove obsolete pubsub method definitions gridcoin-community#2191 (@barton2526)
 - refactor: Remove msMiningErrorsIncluded & msMiningErrorsExcluded gridcoin-community#2215 (@RoboticMind)
 - qt: Remove obsolete topLevelWidget(), Remove obsolete QRegExpValidator gridcoin-community#2198 (@barton2526)
 - net: Drop support of the insecure miniUPnPc versions gridcoin-community#2178 (@barton2526)
 - log: remove deprecated db log category gridcoin-community#2201 (@barton2526)
 - doc: Remove CCT from README and release process docs gridcoin-community#2175 (@barton2526)
 - build: Remove travis references gridcoin-community#2156 (@barton2526)

Fixed
 - gui: Fix macOS and designer font sizes gridcoin-community#2098 (@cyrossignol)
 - gui: Have the TrafficGraphWidget respect the selected stylesheet. gridcoin-community#2107 (@jamescowens)
 - gui: Fix macOS display inconsistencies gridcoin-community#2106 (@cyrossignol)
 - gui: Fix RPC console auto-complete background color gridcoin-community#2108 (@cyrossignol)
 - gui: Avoid reloading redundant stylesheets gridcoin-community#2109 (@cyrossignol)
 - gui: Fix "no active beacon" status gridcoin-community#2110 (@cyrossignol)
 - gui: Fix dark theme link text color visibility gridcoin-community#2115 (@cyrossignol)
 - scraper, util, qt: Fix several deprecations and warnings gridcoin-community#2131 (@jamescowens)
 - gui: Fix duplicate time in GUIUtil::dateTimeStr() gridcoin-community#2139 (@cyrossignol)
 - gui: Fix debug console traffic graph legend colors gridcoin-community#2142 (@cyrossignol)
 - gui: Fix nomenclature gridcoin-community#2104 (@jamescowens)
 - doc: Fix Typos gridcoin-community#2149 (@barton2526)
 - doc: Fix "master" branch build status badge in readme gridcoin-community#2167 (@cyrossignol)
 - gui: Fix Inter font rendering on Windows with FreeType gridcoin-community#2169 (@cyrossignol)
 - gui: Fix assert on non-existent data directory and GUI datadir chooser corner case issues gridcoin-community#2174 (@jamescowens)
 - gui: Fix display artifact in poll loading indicator gridcoin-community#2180 (@cyrossignol)
 - rpc, logging: Minor fixes for sidestake logging gridcoin-community#2187 (@jamescowens)
 - gui: Fix fractional scaling for dialog sizes gridcoin-community#2189 (@cyrossignol)
 - doc: Random fixes gridcoin-community#2197 (@barton2526)
 - doc: getbalance should say GRC not "btc" gridcoin-community#2199 (@barton2526)
 - net: Add missing verification of IPv6 address in CNetAddr::GetIn6Addr¦ gridcoin-community#2200 (@barton2526)
 - doc: remove duplicate line from .gitignore gridcoin-community#2202 (@Pythonix)
 - util: Tweak exception handling in MilliTimer class to eliminate compiler warnings gridcoin-community#2233 (@jamescowens)
 - depends: patch missing include in qt gridcoin-community#2234 (@div72)
 - wallet, rpc: Check each input for IsMine() in GetAddressGroupings gridcoin-community#2242 (@jamescowens)
 - util, qt: Fix snapshot download gridcoin-community#2246 (@jamescowens)
 - gui: Fix Column Widths in RPC Console. Elide long strings in their center. Indent user agent. gridcoin-community#2241 (@barton2526)
 - qt: Fix crash during download snapshot on macOS gridcoin-community#2250 (@jamescowens)
 - qt: Don't allow to open the debug window during splashscreen & verification state gridcoin-community#2245 (@barton2526)
 - gui: Fix address book selected model record when editing gridcoin-community#2253 (@cyrossignol)
 - researcher: Check wallet status before beacon renewal gridcoin-community#2254 (@cyrossignol)
 - qt: Prevent pasting (no label) as label in consolidation transaction gridcoin-community#2255 (@jamescowens)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamescowens jamescowens jamescowens approved these changes

Assignees

@denravonska denravonska

Labels
enhancement gui
Projects
None yet
Milestone
Ingrid
Development

Successfully merging this pull request may close these issues.

Optional Confirmation of Wallet Closure/Exit/Shutdown
2 participants
@denravonska @jamescowens