Toggle hide #855

pull sje397 wants to merge 1 commits into bitcoin:master from sje397:ToggleHide changing 5 files +89 −43
  1. sje397 commented at 2:40 PM on February 17, 2012: contributor 
    Toggle main window hide on tray icon click

- converted openBictoinAction to toggleHideAction
- put GUIUtil functions into a namespace instead of a class
- put window-related functions together in optionsdialog

Reasoning:
- toggle is more typical behaviour
- it's more functional
- better UX

The typical issue with toggling visibility is that when a window
is obscured by other windows but in the 'shown' state, hiding it
isn't what you want. I've added an 'isObscured' function to GUIUtil
that checks several pixels in the window to see if they are visible
on the desktop so that an obscured but shown window can be raised.
  2. laanwj commented at 11:13 AM on February 18, 2012: member

    Is toggling more typical behaviour? I haven't seen that many applications that toggle when the tray icon is clicked, most just appear the window (and bring it to the foreground in some cases).

    Then again, this may be a Windows thing mainly.

  3. sje397 commented at 11:30 AM on February 18, 2012: contributor

    You might be right. I just checked a couple of apps on my wife's windows machine - skype, utorrent - neither toggle. Maybe it was more common a while back. I think it's more common for tools like instant messaging apps that are often used and live on the desktop but need to also get out of the way for a large percentage of time.

    I'd still argue that it's better than the click doing nothing and the icon simply wasting space there. Also it's better UX - you can click the tray icon for a quick glance at your wallet and easily click again without moving the mouse to get back to what you were doing.

    One final point - this patch also fixes the issue whereby if the bitcoin window is oscured by another window, currently clicking the taskbar icon does nothing. It can be annoying when you wonder if it's broken before you notice the task bar item. It should at least raise the window to the front. That part of the patch could be implemented without the toggle.

  4. in src/qt/bitcoingui.cpp:None in d1047c0954 outdated 
     422 | @@ -422,6 +423,16 @@ void BitcoinGUI::showNormal()
 423 |      QMainWindow::showNormal();
 424 |  }
 425 |  
 426 | +void BitcoinGUI::toggleHidden()
 427 | +{
 428 | +    if(isHidden() || isMinimized() || GUIUtil::isObscured(this)) {
 429 | +        showNormal();
    laanwj commented at 11:45 AM on February 18, 2012:

    This showNormal will also unmaximize a maximized window-- better to use showNormalIfMinimized from https://github.com/laanwj/bitcoin/blob/2012_02_altminimizetray/src/qt/bitcoingui.cpp#L792

    sje397 commented at 11:56 AM on February 18, 2012:

    Yep ok. With the flakiness of window systems in this respect, perhaps it would be good (and a little more efficient) to make it a little independent, e.g.

    if(isHidden()) show(); else if(isMinimized()) showNormal(); else if(isObscured()) raise(); activateWindow(); // this (sometimes) helps with keyboard focus on Windows

    laanwj commented at 11:58 AM on February 18, 2012:

    Yes, that'd be better.

  5. laanwj commented at 11:56 AM on February 18, 2012: member

    Seems like useful features, if we can manage to make them work (or, at least not get in the way) on all OSes/permutations.

    Let's first get the #853 working everywhere.

    BTW I agree with changing GUIUtil to a namespace. This would allow splitting it into multiple files when it gets too large.

  6. sje397 commented at 11:59 AM on February 18, 2012: contributor

    Sounds like a good plan to me. I have tested this on KDE and Windows.

  7. sje397 commented at 12:35 PM on February 27, 2012: contributor

    I changed that 'toggleHide' method as per our discussion.

  8. luke-jr commented at 8:59 PM on February 27, 2012: member

    This needs to be rebased :(

  9. sje397 commented at 10:27 AM on February 28, 2012: contributor

    Rebased.

  10. luke-jr commented at 3:11 AM on March 14, 2012: member

    This definitely feels more intuitive to me.

  11. luke-jr commented at 4:02 PM on March 20, 2012: member

    Bug report...

    1. Minimize Bitcoin-Qt (with Minimize to Tray enabled)
    2. Click tray icon

    Actual: Bitcoin-Qt appears and disappears instantly

    Expected: Bitcoin-Qt appears, and remains

  12. laanwj commented at 4:11 PM on March 20, 2012: member

    Ok, that's it, I'm going to remove minimize to tray.

  13. Diapolo commented at 4:18 PM on March 20, 2012: none

    Close to tray remains?

  14. laanwj commented at 4:19 PM on March 20, 2012: member

    Yes. That always worked fine.

  15. Diapolo commented at 5:10 PM on March 20, 2012: none

    Okay then, I guess it would be bad style to leave it in on Windows? Only problem I observed there is, that after a minimize to tray and a window restore, the title-bar looks smaller and only has a close icon instead of the normal minimze, maximize and close icons. Is that indended?

  16. laanwj commented at 5:55 PM on March 20, 2012: member

    No, that was not intended. Pull #941 was supposed to solve it, but seems to have new problems. We've had three or four different implementations now, I've wasted enough time on it.

  17. Diapolo commented at 6:02 PM on March 20, 2012: none

    ACK for removing it, coding time can be better invested for sure!

  18. luke-jr commented at 5:42 AM on March 21, 2012: member

    My aforementioned bug might not be related to this. I am unable to reproduce it with today's next-test... :/

  19. luke-jr commented at 5:56 AM on March 21, 2012: member

    Looks like that bug goes away when this is merged on top of #941 (which is now in master).

  20. Diapolo commented at 6:21 AM on March 21, 2012: none

    How is the general developer policy? Does it really make sense to report these kind of bugs with unofficial client versions? If I'm right you were using a self compiled version of your next-test branch.

  21. luke-jr commented at 6:33 AM on March 21, 2012: member

    Part of the point of pull requests is to test the proposed change... so yes.

  22. laanwj commented at 6:39 AM on March 21, 2012: member

    @diapolo it makes sense when you're testing a certain pull request to post about your experiences with it (as in this case). But of course not to make a new issue about something that isn't in master yet.

    I, at first, didn't notice luke-jr was posting to an existing pull request instead of a new issue, hence the confusion with another commit.

  23. Diapolo commented at 6:42 AM on March 21, 2012: none

    As you perhaps observed I'm trying to help out here and there and to learn how you guys work, I have to ask some "weird questions" from time to time :).

  24. luke-jr commented at 1:32 PM on April 10, 2012: member

    Rebasing required.

  25. sje397 commented at 1:42 PM on April 10, 2012: contributor

    I did that once already.

  26. luke-jr commented at 1:53 PM on April 10, 2012: member

    Unfortunately, I can't rebase others' stuff or I would. It's pretty trivial in this case.

  27. sje397 commented at 1:56 PM on April 10, 2012: contributor

    Yep, I'm doing it now. I'm just suggesting that perhaps you might not want to request a rebase too often, or maybe wait until there are a few people ready to review? It's a waste of time otherwise.

  28. Toggle main window hide on tray icon click
    - converted openBictoinAction to toggleHideAction
- put GUIUtil functions into a namespace instead of a class
- put window-related functions together in optionsdialog

Reasoning:
- toggle is more typical behaviour
- it's more functional
- better UX

The typical issue with toggling visibility is that when a window
is obscured by other windows but in the 'shown' state, hiding it
isn't what you want. I've added an 'isObscured' function to GUIUtil
that checks several pixels in the window to see if they are visible
on the desktop so that an obscured but shown window can be raised.

Conflicts:

	src/qt/guiutil.cpp
	src/qt/guiutil.h
    86d5634941
  29. sje397 commented at 2:53 PM on April 10, 2012: contributor

    Rebased. It does conflict a little bit with #961, only because both modify the options dialog.

  30. laanwj commented at 6:57 PM on April 11, 2012: member

    I've tested it, it works for me (on Linux). Code is also OK with me.

    ACK

  31. luke-jr commented at 7:03 PM on April 11, 2012: member

    ACK here too, been using this for months now.

  32. laanwj referenced this in commit 4ac24cf59e on Apr 12, 2012
  33. laanwj merged this on Apr 12, 2012
  34. laanwj closed this on Apr 12, 2012
  35. coblee referenced this in commit 827620a22e on Jul 17, 2012
  36. destenson referenced this in commit 284e2c41cc on Jun 26, 2016
  37. dexX7 referenced this in commit 5ecff7a9a2 on Jan 28, 2019
  38. DrahtBot locked this on Sep 8, 2021
Contributors
sje397laanwjluke-jrDiapolo
Labels
GUI
Linked (view graph)
#961 Remove "minimize to tray"
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: 2026-04-13 18:16 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me