71343 - [BEOS] Windows are not brought to front when requested

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Gregor Rosenauer]

When you select "Manage bookmarks" and the bookmark-manager is hidden by other
windows, it should be brought to front to make it visible again.
See also  #68550.

Comment 1

[timeless]

iirc there is either an easy way to do this or a bug for it in general

Comment 2

[Claudius Gayle]

I can confirm this with a 2001030810 Linux build but I'm pretty sure this is a dupe of a
longstanding (possibly xptoolkit) bug. I'm pretty sure you'd get the same behavior w/
any other window/xpapp on linux.

Comment 3

[Claudius Gayle]

I believe that (after much reading) it can be shown that this bug is a dupe of bug 8002.
Anyone else agree? I'm looking for a 2nd opinion so I don't both mark it and verify it (if
I turn out to be wrong)

Comment 4

[timeless]

yeah, Comments From [e-mail] 1999-08-09 10:15 is what i was thinking 
about
dupe of nsIWidget::Show needs to bring the window to the front on linux

*** This bug has been marked as a duplicate of 8002 ***

Comment 5

[Claudius Gayle]

motion passed. VERIFIED Dupe

Comment 6

[Sergei Dolgov]

Bug Still here. And not only for bookmark manager.
Investigating - I already implemented Behind nethod in nsWindow, dut it didn't
affect problem - maybe we should really process aRaise==PR_TRUE case in SetFocus
or change Show() implementation (by adding !IsFront to IsHidden case)

Comment 7

[Sergei Dolgov]

forgot cc myself

Comment 8

[Kai Lahmann (is there, where MNG is)]

seams to be BeOS-only, at least it doesn't appear on Linux.

Comment 9

[Patrick Xia]

(2002071813) maybe it was fixed sometime, maybe it was never there.

-> WFM

Comment 10

[Sergei Dolgov]

Octalc0de - do you mean BeOS Mozilla????
Since i started using it, it never worked (BeOS Mozilla build) in this sense.
Nor by clicking on ComponentBar neither with choosing component from Winow Menu.

Paul, does it work for you on nightly builds? Of not, that WORKSFORME don't count.

Comment 11

[Sergei Dolgov]

BeOS nsWidget Show() needs rewrite. It calls frontmosting function
(BWindow:Show()) for BWindow only if it was hidden. But visible-non-active
window behind active window isn't "hidden" in BeOS sense.

Comment 12

[Paul]

This is more than just the "Bookmark Manager".  Requesting a window from the "Windows"
menu, does not bring that window forward, either.  I think Sergei is correct, in saying
that the nsWindow::Show() needs a re-write.

Comment 13

[Paul]

Assigning to Sergei, since he has been working focus issues under BeOS

Comment 14

[Sergei Dolgov]

Setting dependence. Ability and method to control Z-order of BWindows heavily
depends on window_feel. No sense to patch this bug before some decision on on
checkin of patch for bug 66809.

Comment 15

[Sergei Dolgov]

Attachment: Patch - fixes several problems

Besides main problem, it fixes half of bug 95348 and prevents from some side
effects when patch for bug 66809 will be commited (comment
http://bugzilla.mozilla.org/show_bug.cgi?id=66809#c32)

Comment 16

[Sergei Dolgov]

Kai, Paul - can you verify it for BeOS functionality for your vanilla builds?
Problem is that I already have lot of patches applied, and wonder how it works
for "normal" builds. (timeless is unfortunately still out of BeOS game )

Comment 17

[Kai Lahmann (is there, where MNG is)]

Sergey: would you call the "5.04 Dev Edition" a vanilla builds? ;)

Comment 18

[Sergei Dolgov]

Attachment: typo fix

added missing '}'

Comment 19

[Sergei Dolgov]

Paul, if and when you test it, if you got blank menu - comment out those lines:
//if (mBorderlessParent && mBorderlessParent->Window() &&
!mBorderlessParent->Window()->IsActive())
//  return NS_OK;
Those lines are intended to work with my code which handles popups differently -
and may affect visibility of some menus in your case. But maybe it don't.

Comment 20

[Paul]

Comment on attachment 92513 [details] [diff] [review]
typo fix

First thing, functionally this works.  Good Job!!

A couple of things, however:
1- Please use tab indenting, instead of spaces
2- mustunlock is not needed
change:
>-	if(mView)
>-	{
>-		if(mView->LockLooper())
>-			mustunlock = true;
to:
  if (mView && mView->LockLooper()) {

3- havewindow is not needed, and should not be checked by looking to see if the
view has a Parent, as it may be a child of another view.  Instead, just change
all references of havewindow to mView->Window()

example:

>-			if(havewindow && !mView->Window()->IsHidden())

would become:
if (mView->Window() && !mView->Window()->IsHidden())

4- I know I may be nitpicking, but, could you indent your code within the case
statement for readability?
you have:
  case: something
  {
  }
  break;
  case: something_else
please do it like:
  case: something
    {
    }
    break;
  case: something_else


Other than that, like I said, good work.

Comment 21

[Sergei Dolgov]

1)About Locking. I don't wish to remove it now to put it back  with next patch
about popups. Though, will check this possibility once more.

2)Identing - there were big "wars" with timeless. I'm afraid he don't pass me
through, if notice any tab in my code :). Maybe i will correct parenthesis
identation for case: - but i used same idea as for if. Though, no problem.

3) Seems you are right about havewindow, i thought about it and didn't remove
only for safety - who knows. But if it will be tested bit more - i remove it.

Heh, and do not ask for "generalising" code - e.g. with joining all
BView->Hide/Show in one place - who knows, what will happen. Maybe hiding View
is senseless for PopUps...Maybe we will add cases for other eWindow types.

Comment 22

[Paul]

In response:
1) What "next patch about popups"?
2) The header at the top of the file says:
/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
So, please indent with tabs, especially since the rest of the file is this way :)
If timeless doesn't like it, he should change the header and the indenting of the entire 
file :)  Really, the default editor on BeOS is BeIDE, and it uses tabs for indenting, which 
is another reason for tabs in BeOS specific code.  But, watch your step elsewhere in the 
tree, as not all files in mozilla are tab-indented :)
3) cool.
And, the switch case is fine, for the reasons you stated :)

Comment 23

[:deanis74]

Doesn't "indent-tabs-mode: nil" mean don't use tabs to indent?

Comment 24

[Paul]

oops, didn't notice the "nil".  Anyway, the rest of the file uses tabs.

Comment 25

[Sergei Dolgov]

Sure, i also prefer tabs and using spaces for me is pure pain with BeIDE.
Especially if we have more than one level of identation in function :)

Comment 26

[Sergei Dolgov]

Attachment: Patch. Cleanup

Paul, i hope you'll test it soon. This really needs testing - maybe there are
hidden issues - lot of changes.

Removed extra function calls - it seems that no need for hiding mView in
deactivated toplevel/dialog window. Though, BWindow::Show() and BView::Show()
are still here - for first run purpose.
Also seems no need, as i mentioned, to additional hiding of mView in hidden
popups/BWindows.

Changed switch() styling - according to big switch in CallMethod().
About havewindow/mustlook. Same style is used by nunerous other methods in BeOS
nsWindow code. So it is still here. And it looks reasonable for me. But maybe
i'll chane my opinion.

Spaces and tabs - huh..no decision yet. Mozilla rules agains our predecessor
style and BeIDE :(

Comment 27

[Sergei Dolgov]

btw. with this code i can't get rid of aggressive Mozilla behaviour - putting
itself frontmost when page download is finished. Dunno if it presents on other
platforms

Comment 28

[Sergei Dolgov]

Attachment: Removing autofrontmost issue

It seems i did the trick. Activation from Deskbar, ComponentBar and Window menu
is still working, but no autofrontmosting more on foreground downloads.

Comment 29

[Paul]

Attachment: updated patch with changes mentioned earlier

Sergei, this is your last patch, with some minor tweaks:
1- Updated first line, so emacs users will not uses spaces for indenting :)
2- Updated new code to indent with tabs, like the rest of the file :)
3- removed mustunlock and havewindow variables from nsWindow::Show()
4- Now checks for existence of window using mView->Window()
5- Check to make sure we are attached to a window (just in case) in default
section of the switch statement (avoid possible segfault)

Comment 30

[Paul]

Sergei, if you are ok with my changes, I'll give this my r=

Comment 31

[Sergei Dolgov]

seems ok visually. Did you test it already? (cannot test it myself just now)
Btw, i have feeling that with some of my last changes here also tooltips started
working better a bit.

Comment 32

[Paul]

Comment on attachment 92841 [details] [diff] [review]
updated patch with changes mentioned earlier

r=arougthopher

needs a= for checkin

as for tooltips, they still cause the window to be brought forward, but, that
is another bug.

Comment 33

[Sergei Dolgov]

Paul, until i submit another patch, you can use in your internal builds
  if (mBorderlessParent && mBorderlessParent->Window() &&
!mBorderlessParent->Window()->IsActive())
     return NS_OK;
in begginning of Show() method to avoid driving Mozilla to front by tooltips

Comment 34

[Asa Dotzler [:asa]]

Comment on attachment 92841 [details] [diff] [review]
updated patch with changes mentioned earlier

a=asa (on behalf of drivers) for checkin to 1.1

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 36

[Sergei Dolgov]

after digging Bonsai down to 1999 i figured out that method, responsible for
this feature is SetFocus(aRaise) with aRaise==PR_TRUE. Not Show(). Good piece of
humor. Though, implementation of the feature via SetFocus was impossible before,
because it required different way in handling activation events than used in
BeOS   port. I rewrote here this activation/focus handling and draft version
seems very good in all sences. Though, only problem is BeOS API issue with
handling window Z-order, which results in several problems when B_MODAL windows
are in use everywhere (except, maybe, popups). So, there will be changes also
for bug 66809

Comment 37

[Sergei Dolgov]

really fixed. Mostly due Activation in SetFocus()