Closed
Bug 313403
Opened 17 years ago
Closed 13 years ago
nsWindow::GetAttention should obey the system set FOREGROUNDFLASHCOUNT if aCycleCount = -1 (background dialogs flash in the taskbar forever, should only flash <user-specified> times)
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1a2
People
(Reporter: shayne.anthony.jewers, Assigned: emaijala+moz)
References
()
Details
Attachments
(1 file, 5 obsolete files)
10.71 KB,
patch
|
neil
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051021 Firefox/1.5 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051021 Firefox/1.5 When FF, TB or XR wants to get my attention with window.getAttention(), it flashes on the windows taskbar instead of my specified 1 flash Reproducible: Always Steps to Reproduce: 1.Using TweakUI, in Generak>focus, set "Flash Taskbar button" to 1 2.get FF, TB or XR to get your attention (eg. "Do you want to set FF as your default broswer?" box 3.Not the taskbar Actual Results: The Taskbar flashes forever Expected Results: It should have flashed once, like I set in TweakUI
Comment 1•17 years ago
|
||
Even better would be to use FlashWindowEx for the win32 impl of getAttention instead of FlashWindow, eliminating the need to do timer stuff ourselves. According to http://msdn.microsoft.com/library/default.asp?url=/library/en-us/debug/base/flashwindowex.asp, FlashWindowEx is supported in everything >= Windows 98, so unless Windows 95 support is a problem this could simplify code quite a bit (eliminate the need for nsAttentionTimerMonitor). Using the user-specified pref would also be easier, but I haven't looked into how to get that value.
Comment 2•17 years ago
|
||
Looks like you can get the appropriate blink time using SystemParametersInfo(SPI_GETFOREGROUNDFLASHCOUNT), but not in Win95 or WinNt (dwMajorVersion >= 5 || dwMinorVersion >= 10).
Summary: Windows Flash Forever instead of The User-Specified Amount of times → nsWindow::GetAttention should obey the system set FOREGROUNDFLASHCOUNT if aCycleCount = -1 (background dialogs flash in the taskbar forever, should only flash <user-specified> times)
Reporter | ||
Updated•16 years ago
|
Flags: blocking1.8.1?
Version: Trunk → 1.8 Branch
Reporter | ||
Updated•14 years ago
|
Component: Widget: Win32 → XP Toolkit/Widgets: Menus
Version: 1.8 Branch → Trunk
Assignee | ||
Updated•14 years ago
|
Assignee: win32 → emaijala
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #332086 -
Flags: review?(timeless)
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•14 years ago
|
Attachment #332086 -
Flags: review?(timeless) → review?(timeless)
Attachment #332086 -
Flags: review?(timeless) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #332086 -
Flags: superreview?(roc)
Attachment #332086 -
Flags: superreview?(roc) → superreview+
Comment 5•14 years ago
|
||
pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/7a55d41f8ca2
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 6•14 years ago
|
||
Backed out due to build bustage: c:/builds/slave/trunk_win2k3_hw/build/widget/src/windows/nsWindow.cpp(7353) : error C2446: '==' : no conversion from 'HWND' to 'HWND (__stdcall *)(void)' There is no context in which this conversion is possible c:/builds/slave/trunk_win2k3_hw/build/widget/src/windows/nsWindow.cpp(7353) : error C2040: '==' : 'HWND (__stdcall *)(void)' differs in levels of indirection from 'HWND'
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 7•14 years ago
|
||
Fixed typo in the first patch. Transferring reviews.
Attachment #332086 -
Attachment is obsolete: true
Attachment #334547 -
Flags: superreview+
Attachment #334547 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Comment 8•14 years ago
|
||
Pushed as 17104:4460de8869d8.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: Checkin needed after alpha2
Target Milestone: --- → mozilla1.9.1a2
Updated•14 years ago
|
Component: XP Toolkit/Widgets: Menus → Widget: Win32
In the latest nightly build, with this patch, alert boxes now flash the firefox taskbar item even if it is in the foreground. In the previous nightly this doesn't happen.
Backed out to avoid getting it into 1.9.1a2
Assignee | ||
Comment 12•14 years ago
|
||
Comment on attachment 334547 [details] [diff] [review] Patch v1.1 Of course, to make this work properly the flashing needs to be canceled when focus is obtained. To do that we need to fix how dialogs are displayed because they think they have focus right away (with flashing disabled they even look like that).
Attachment #334547 -
Attachment is obsolete: true
Assignee | ||
Comment 13•14 years ago
|
||
A new try. Adding a check so we don't flash if the owner is the foreground window and stop flashing upon activation. Plus don't try to activate dialogs if that's not allowed.
Attachment #335935 -
Flags: review?(neil)
Comment 14•14 years ago
|
||
Comment on attachment 335935 [details] [diff] [review] Patch v1.2 Does the GetAttention(2); call inside nsWindow::Show need to be changed to -1? >- if (!sIsInEndSession) >- ::ShowWindow(mWnd, SW_HIDE); >+ if (!sIsInEndSession) >+ ::ShowWindow(mWnd, SW_HIDE); Not sure what happened here? >+ StopFlashing(); Can you not use FLASHW_TIMERNOFG to avoid this? (It's not a panacea because it will unfortunately still try to flash the active window once.) >+ FLASHWINFO flashInfo; >+ ZeroMemory(&flashInfo, sizeof(FLASHWINFO)); >+ flashInfo.cbSize = sizeof(FLASHWINFO); >+ flashInfo.hwnd = flashWnd; >+ flashInfo.dwFlags = FLASHW_ALL; >+ flashInfo.uCount = aCycleCount > 0 ? aCycleCount : defaultCycleCount; Since you're setting nearly all the members anyway wouldn't an initialiser be more efficient?
Assignee | ||
Comment 15•14 years ago
|
||
(In reply to comment #14) > (From update of attachment 335935 [details] [diff] [review]) > Does the GetAttention(2); call inside nsWindow::Show need to be changed to -1? I'd say no. We just want a quick note that the window didn't come to foreground and if the system parameter is long it would be quite annoying. > >- if (!sIsInEndSession) > >- ::ShowWindow(mWnd, SW_HIDE); > >+ if (!sIsInEndSession) > >+ ::ShowWindow(mWnd, SW_HIDE); > Not sure what happened here? Leftover from another patch though I don't know how that got there with no real change.. > >+ StopFlashing(); > Can you not use FLASHW_TIMERNOFG to avoid this? (It's not a panacea because it > will unfortunately still try to flash the active window once.) That would have been nice, but as we flash the owner, that wouldn't be the foreground window. > >+ FLASHWINFO flashInfo; > >+ ZeroMemory(&flashInfo, sizeof(FLASHWINFO)); > >+ flashInfo.cbSize = sizeof(FLASHWINFO); > >+ flashInfo.hwnd = flashWnd; > >+ flashInfo.dwFlags = FLASHW_ALL; > >+ flashInfo.uCount = aCycleCount > 0 ? aCycleCount : defaultCycleCount; > Since you're setting nearly all the members anyway wouldn't an initialiser be > more efficient? Well, I believe in ZeroMemory just to make sure the struct is properly initialized even if it gets new members in a future version.
Status: REOPENED → ASSIGNED
Comment 16•14 years ago
|
||
(In reply to comment #15) >(In reply to comment #14) >>>+ StopFlashing(); >>Can you not use FLASHW_TIMERNOFG to avoid this? (It's not a panacea because it >> will unfortunately still try to flash the active window once.) >That would have been nice, but as we flash the owner, that wouldn't be the >foreground window. I guess it must be an undocumented feature of FLASHW_TIMERNOFG that it works even when the flashed window is the owner of the foreground window. >>>+ FLASHWINFO flashInfo; >>>+ ZeroMemory(&flashInfo, sizeof(FLASHWINFO)); >>>+ flashInfo.cbSize = sizeof(FLASHWINFO); >>>+ flashInfo.hwnd = flashWnd; >>>+ flashInfo.dwFlags = FLASHW_ALL; >>>+ flashInfo.uCount = aCycleCount > 0 ? aCycleCount : defaultCycleCount; >>Since you're setting nearly all the members anyway wouldn't an initialiser be >>more efficient? >Well, I believe in ZeroMemory just to make sure the struct is properly >initialized even if it gets new members in a future version. And I believe in C initialiser semantics to make sure the struct is properly initialised even if it gets new members in a future version.
Assignee | ||
Comment 17•14 years ago
|
||
(In reply to comment #16) > I guess it must be an undocumented feature of FLASHW_TIMERNOFG that it works > even when the flashed window is the owner of the foreground window. Could be that I confused it with the forementioned single flash when testing. Will try again. > And I believe in C initialiser semantics to make sure the struct is properly > initialised even if it gets new members in a future version. So I failed the semantics awareness test...
Assignee | ||
Updated•14 years ago
|
Attachment #335935 -
Attachment is obsolete: true
Attachment #335935 -
Flags: review?(neil)
Assignee | ||
Comment 18•14 years ago
|
||
FLASHW_TIMERNOFG seems to work fine. One remaining problem is that the first time an alert is displayed it will bring the window to foreground no matter what. Still need to find out the reason for this.
Assignee | ||
Comment 19•13 years ago
|
||
Finally... The foreground change first time a dialog was displayed seems to be some kind of a debugger interaction, as it doesn't happen outside Visual Studio. And even there it doesn't steal focus.
Attachment #368772 -
Flags: review?(neil)
Comment 20•13 years ago
|
||
Comment on attachment 368772 [details] [diff] [review] Patch v1.3 Sorry to mess you around like that, but it seems that TIMERNOFG refers to the application, not the window :-(
Attachment #368772 -
Flags: review?(neil) → review-
Updated•13 years ago
|
Attachment #335935 -
Attachment is obsolete: false
Attachment #335935 -
Flags: review+
Assignee | ||
Comment 21•13 years ago
|
||
Comment on attachment 335935 [details] [diff] [review] Patch v1.2 Oh, no worries. I didn't notice that either. Annoying, but I'll update the previous patch to actually apply and change the struct initialization while at it.
Attachment #335935 -
Attachment is obsolete: true
Attachment #335935 -
Flags: review+
Assignee | ||
Comment 22•13 years ago
|
||
Ok, this is the previous patch renewed.
Attachment #368772 -
Attachment is obsolete: true
Attachment #369331 -
Flags: review?(neil)
Assignee | ||
Comment 23•13 years ago
|
||
Ok, this is the previous patch renewed.
Attachment #369331 -
Attachment is obsolete: true
Attachment #369339 -
Flags: review?(neil)
Attachment #369331 -
Flags: review?(neil)
Updated•13 years ago
|
Attachment #369339 -
Flags: review?(neil) → review+
Assignee | ||
Updated•13 years ago
|
Attachment #369339 -
Flags: superreview?(roc)
Attachment #369339 -
Flags: superreview?(roc) → superreview+
Comment on attachment 369339 [details] [diff] [review] Patch v1.2 renewed + while (HWND ownerWnd = ::GetWindow(flashWnd, GW_OWNER)) + flashWnd = ownerWnd; {} around the statement
Assignee | ||
Comment 25•13 years ago
|
||
Pushed e783d41a4e4a to mozilla-central with roc's comment noted.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•13 years ago
|
||
I ifndef'd the flashwindow stuff from WinCE to fix bustage.
Comment 27•11 years ago
|
||
Comment on attachment 369339 [details] [diff] [review] Patch v1.2 renewed >+ HWND flashWnd = mWnd; >+ while (HWND ownerWnd = ::GetWindow(flashWnd, GW_OWNER)) >+ flashWnd = ownerWnd; >+ >+ // Don't flash if the owner window is active either. >+ if (fgWnd == flashWnd) >+ return NS_OK; The old code may have had this bug too, but it's easier to see like this ;-) If the flash window is the parent of the foreground window which is the parent of the attention window, then I don't think we want to flash. I'm not so sure about the case where the attention window and the foreground window have the same owner.
You need to log in
before you can comment on or make changes to this bug.
Description
•