Qt: Enable system tray icon by default if available #14228

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:unavailable-systray changing 2 files +17 −3
  1. hebasto commented at 8:11 pm on September 15, 2018: member

    Prevent a user from losing access to the main window by minimizing it to the tray on the systems which have not “system tray” or “notification area” available (e.g. GNOME 3.26+).

    Tested on Fedora 28 + GNOME 3.28.

  2. fanquake added the label GUI on Sep 16, 2018
  3. practicalswift commented at 6:32 am on September 16, 2018: contributor

    Original PR title: “Qt: Don’t use systray icon on inappropriate systems”

    When reading the PR title I first thought you were referring to Windows platforms :-)

  4. in src/qt/optionsmodel.cpp:58 in 34c1b55c43 outdated 
    53@@ -53,12 +54,16 @@ void OptionsModel::Init(bool resetSettings)
54     // These are Qt-only settings:
55 
56     // Window
57-    if (!settings.contains("fHideTrayIcon"))
58+    if (!QSystemTrayIcon::isSystemTrayAvailable()) {
59+        settings.setValue("fHideTrayIcon", true);
    luke-jr commented at 10:00 am on September 16, 2018:
    Kinda of nasty to change the saved setting just because you run it once on a platform that doesn’t support it. I suggest leaving it be in settings, greying the setting, and ignoring it.
  5. luke-jr changes_requested
  6. hebasto force-pushed on Sep 16, 2018
  7. hebasto commented at 1:00 pm on September 16, 2018: member
    @luke-jr Thank you for your review. You’re right. Fixed: the saved settings aren’t touched. Please re-review.
  8. in src/qt/optionsmodel.cpp:58 in b642b6f273 outdated 
    54@@ -54,7 +55,7 @@ void OptionsModel::Init(bool resetSettings)
55 
56     // Window
57     if (!settings.contains("fHideTrayIcon"))
58-        settings.setValue("fHideTrayIcon", false);
59+        settings.setValue("fHideTrayIcon", !QSystemTrayIcon::isSystemTrayAvailable());
    luke-jr commented at 1:59 pm on September 16, 2018:
    I’d leave this alone. Or at the very least, don’t set any value if there’s no systray.
    hebasto commented at 2:28 pm on September 16, 2018:
    If it’ll be leaved alone the first call of the OptionsModel constructor sets fHideTrayIcon to the false anyway. Why cannot it be initialized to the true on the GNOME environment?
    promag commented at 4:42 pm on September 16, 2018:
    I’d say that’s another PR, like “gui: Enable system tray icon by default if available”
    luke-jr commented at 9:31 pm on September 16, 2018:

    @hebasto Because the environment is not necessarily a permanent thing. Someone might run it on GNOME, and then run it on KDE.

    Systems without a systray should just behave as if the option doesn’t exist at all IMO.

  9. luke-jr changes_requested
  10. in src/qt/bitcoingui.cpp:588 in b642b6f273 outdated 
    584@@ -585,6 +585,9 @@ void BitcoinGUI::setWalletActionsEnabled(bool enabled)
585 
586 void BitcoinGUI::createTrayIcon(const NetworkStyle *networkStyle)
587 {
588+    if (!QSystemTrayIcon::isSystemTrayAvailable()) {
    promag commented at 4:47 pm on September 16, 2018:
    I would prevent calls to createTrayIcon and maybe assert(QSystemTrayIcon::isSystemTrayAvailable()) here?
  11. promag commented at 4:47 pm on September 16, 2018: member
    How about createTrayIconMenu?
  12. hebasto commented at 5:32 pm on September 16, 2018: member

    @promag

    Thank you for your review.

    How about createTrayIconMenu?

    https://github.com/bitcoin/bitcoin/blob/c53e083a49291b611d278a8db24ff235c1202e43/src/qt/bitcoingui.cpp#L603-L604

    Is it not enough?

  13. hebasto force-pushed on Sep 16, 2018
  14. hebasto commented at 6:25 pm on September 16, 2018: member
    @promag You’ve suggested a more robust code. Fixed. Please re-review.
  15. hebasto renamed this:
    Qt: Don't use systray icon on inappropriate systems
    Qt: Enable system tray icon by default if available
    on Sep 16, 2018
  16. hebasto commented at 6:31 pm on September 16, 2018: member
    This PR title has been changed as @promag proposed. btw, gui prefix is not mentioned in the CONTRIBUTING.md. Is it valid? (answered by @laanwj on IRC)
  17. hebasto force-pushed on Sep 17, 2018
  18. hebasto commented at 3:16 pm on September 17, 2018: member

    @luke-jr

    Someone might run it on GNOME, and then run it on KDE. Systems without a systray should just behave as if the option doesn’t exist at all IMO.

    You’re right. Fixed: settings are not touched at all on systems without a systray.

  19. laanwj commented at 11:35 am on September 23, 2018: member
    utACK after squash
  20. hebasto force-pushed on Sep 23, 2018
  21. hebasto commented at 4:13 pm on September 23, 2018: member

    utACK after squash @laanwj squashed

  22. jonasschnelli commented at 6:36 pm on September 25, 2018: contributor
    lightly tested on OSes where the tray is available. ACK 244774d9626f37e15d8e021de4e735302ae2d292.
  23. jonasschnelli commented at 6:37 pm on September 25, 2018: contributor
    Waiting on @promag’s ack or further comments
  24. promag commented at 6:43 pm on September 25, 2018: member
    @jonasschnelli thanks for the reminder, will check tonight.
  25. promag commented at 0:02 am on September 26, 2018: member

    Could guard these mappings https://github.com/bitcoin/bitcoin/blob/244774d9626f37e15d8e021de4e735302ae2d292/src/qt/optionsdialog.cpp#L222-L223

    but since these widgets are disabled and the options dialog is the only place that changes these options, the guard is not necessary.

    nit, could fix the comment https://github.com/bitcoin/bitcoin/blob/244774d9626f37e15d8e021de4e735302ae2d292/src/qt/optionsdialog.cpp#L220

    utACK 244774d — tomorrow I can setup a system without tray to test.

  26. hebasto commented at 8:12 pm on September 26, 2018: member

    @promag

    but since these widgets are disabled and the options dialog is the only place that changes these options, the guard is not necessary.

    ~Agree.~ (see below)

    nit, could fix the comment

    I did not catch your idea. What should it be like?

  27. promag commented at 8:17 pm on September 26, 2018: member
    Should be “Non macOS”?
  28. promag commented at 8:17 pm on September 26, 2018: member
    BTW, I’m currently installing fedora..
  29. hebasto commented at 8:28 pm on September 26, 2018: member

    @promag

    Should be “Non macOS”?

    Ah, I see. The comment line /* Window */ just repeats the Options tab name.

  30. promag commented at 9:17 pm on September 26, 2018: member
    @hebasto right, disregard that comment then.
  31. hebasto commented at 1:33 pm on September 27, 2018: member

    @promag

    but since these widgets are disabled and the options dialog is the only place that changes these options, the guard is not necessary.

    In fact, the guard around mapper is necessary: addMapping effectively overwrites ui settings in the OptionsDialog constructor.

    Thank you for this catch. I’m going to fix it.

  32. Don't use systray icon on inappropriate systems 
    Prevent a user from losing access to the main window by minimizing it to
the tray on some systems (e.g. GNOME 3.26+).
    ec1201a368
  33. hebasto force-pushed on Sep 27, 2018
  34. hebasto commented at 2:28 pm on September 27, 2018: member
    @promag’s catch has been fixed.
  35. DrahtBot commented at 9:37 am on September 28, 2018: member
    Coverage Change (pull 14228) Reference (master)
    Lines +0.0287 % 87.0361 %
    Functions +0.1235 % 84.1130 %
    Branches +0.0085 % 51.5451 %
  36. hebasto commented at 7:22 pm on October 13, 2018: member
    @promag Would you mind re-reviewing this PR?
  37. laanwj commented at 2:07 pm on November 12, 2018: member
    utACK ec1201a36847f7aa942eab1b3a3d082f6daf0031
  38. pull[bot] referenced this in commit 5cdfd72b17 on Nov 12, 2018
  39. laanwj merged this on Nov 12, 2018
  40. laanwj closed this on Nov 12, 2018
  41. promag commented at 2:11 pm on November 12, 2018: member
    Sorry @hebasto, missed this. utACK ec1201a.
  42. hebasto deleted the branch on Nov 12, 2018
  43. hebasto referenced this in commit c7a816a7f1 on Dec 19, 2018
  44. hebasto referenced this in commit c8d9d9093b on Dec 19, 2018
  45. luke-jr referenced this in commit e43e1d832a on Dec 24, 2018
  46. MarcoFalke referenced this in commit fb52d0684e on Jan 2, 2019
  47. HashUnlimited referenced this in commit cad10789b4 on Jan 8, 2019
  48. Munkybooty referenced this in commit 1f22bab89a on Jul 29, 2021
  49. Munkybooty referenced this in commit e5e122ebb0 on Jul 29, 2021
  50. Munkybooty referenced this in commit 06d1dfabde on Aug 3, 2021
  51. Munkybooty referenced this in commit 385966fde8 on Aug 3, 2021
  52. Munkybooty referenced this in commit 858f924aa8 on Aug 5, 2021
  53. Munkybooty referenced this in commit b8771d8153 on Aug 8, 2021
  54. Munkybooty referenced this in commit d5372579de on Aug 9, 2021
  55. Munkybooty referenced this in commit 1eb7a514f7 on Aug 10, 2021
  56. Munkybooty referenced this in commit 4f866c72cc on Aug 11, 2021
  57. Munkybooty referenced this in commit a73eb08dca on Aug 11, 2021
  58. Munkybooty referenced this in commit f6b1552e85 on Aug 11, 2021
  59. 5tefan referenced this in commit 05c0fd5408 on Aug 12, 2021
  60. Munkybooty referenced this in commit bf57e76984 on Aug 13, 2021
  61. Munkybooty referenced this in commit 211c3cace6 on Aug 15, 2021
  62. DrahtBot locked this on Sep 8, 2021


hebasto practicalswift luke-jr promag laanwj jonasschnelli DrahtBot

Labels
GUI

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-23 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me