Closed Bug 296856 Opened 16 years ago Closed 15 years ago

[BEOS] nsWindow suggested window creation changes

Categories

(Core Graveyard :: Widget: BeOS, enhancement)

x86
BeOS
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: thesuckiestemail, Assigned: thesuckiestemail)

References

Details

Attachments

(2 files, 10 obsolete files)

User-Agent:       Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.8b2) Gecko/20050604 Firefox/1.0+
Build Identifier: 

Removing and moving some code around. nsWindows now start out with transparent
and relies on BaseWidget to set some variables (border and window type is set in
 StandardWindowCreate anyway). 

OnPaint removed code (wasn't that code obsolete, Sergei) and refactored to get 
away from ifs in ifs ugliness (IMO).

SetBackgroundColor only sets backgroundcolor, the view set itself transparent
when attached to a window instead.

We set the lastViewWidth and height in the constructor from the frame instead. 

Reproducible: Always

Steps to Reproduce:
Attached patch suggested cleanups (obsolete) — Splinter Review
Sergei, I went thru my experimental nsWindow, and I think these might be worth
adding. What do you think?
Added the StandardWindowCreate I wrote, which is dependent on the other changes
herein. Otherwise it blinks so it's probably fixed when we set views
transparent.
Assignee: beos → thesuckiestemail
Attachment #185490 - Attachment is obsolete: true
Status: NEW → ASSIGNED
The new StandardWindowCreate is dependent on bug 296823 as it creates fewer views.
Depends on: 296823
good step in general. Will investigate it bit more.
It's needed as I have a few questionmarks. Even though I think it works good
enough, as it has been tested in quite some months in my test-releases.

When I first tested it it shaved of 2 seconds in startup-time, but now I have a
faster laptop with slower harddrive so it's mostly harddrive-dependent.
Changed title to more reflect the StandardWindowCreate changes.
Summary: [BEOS] nsWindow suggested minor cleanups → [BEOS] nsWindow suggested window creation changes
some of the changes proposed in the original patch have crept into CVS through
other means.  Just updating the patch to match CVS.
actually i wish we leave there at least comment about "invisible" case, and use
Run() for BLooper, unrelated to windows type. I still have feeling that we
cannot rely on Mozilla's nsWidget::Show() in our case, in order to keep message
chain consistent.
Patch derived from CVS after clearing of dependent bug.  Feedback appreciated
(so we can keep things moving).
Attachment #195873 - Attachment is obsolete: true
Attachment #195957 - Flags: review?(sergei_d)
i need to test if re-casting of nsViewBeOS and also regarding changes in *.h
file are done. Also parenthesis identation isn't unified in current patch. Also
some comments may be added, so after testing patch this evening i probably
contact Doug in private
Only comment I have is on OnPaint, as I don't like long if() blocks the 
'if((r.width || r.height) && mEventCallback)'-line can be refactored and
combined with the 'if(!mView)'-line to return instead.
fyysik, where did we leave off on this?  Seems like we'll need an updated patch
after landing scrolling changes in bug 129310.  Is this something I should
create or will you create one after addressing tqh's concerns in comment #12? 
It'd be good to land this one, too, I think.
Depends on: 192310
Doug, i think this is now tqh's deal. As he added some more changes, quite
logical, to StandardWindowCreate.
Don't rush. I try to discuss it with him, and anyway, scrolling patch isn't in
the tree.
I think we should be able to do a better patch once the scrolling is done. This
has mostly been delayed because it hasn't been discussed and tested properly,
but with the scrolling changes that's no longer the case.

Doug, keep up the good work.
The next patch will probably depend on bug 310090
Depends on: 310090
Doug, what is this dependency on 192310?
Ok, saw the error in the dependency.
Depends on: 129310
No longer depends on: 192310
Removed a lot of unused code, changed a couple of bools to packedbools. 
There doesn't seem to be a reason for us to have our own setBackgroundColor and
setForegroundColor especially with the lockingoverhead.
Also fixed some warnings.

Note my comment in StandardWindowCreate when a childview is created but without
parent. That might need investigation before we commit this.
Attachment #185493 - Attachment is obsolete: true
Attachment #195957 - Attachment is obsolete: true
Blocks: 266252
Attached file patch breaks startup (obsolete) —
latest patch applies clean, builds OK but firefox won't start.	Attachment
includes stack crawl and console output.  Note the "NO PARENT" warning in the
console output right before it dies.
Comment on attachment 197471 [details]
patch breaks startup

problem caused by dependency on 310090.
Attachment #197471 - Attachment is obsolete: true
I've noticed quite a bit of console debug output in the form 

###!!! ASSERTION: invalid break array: 'aBreaks[aNumBreaks - 1] == aLength',
file /boot/home/develop/mozilla/gfx/src/beos/nsRenderingContextBeOS.cpp, line 1151
Break: at file
/boot/home/develop/mozilla/gfx/src/beos/nsRenderingContextBeOS.cpp, line 1151   

An early workaround from fyysik eliminated this message.  While we're cleaning
things, can we also take care of this message since it seems to be irrelevant?
We might, but it's in gfs/src/beos and has not very much in common with this, so
maybe it should be fixed on it's own.
Doug, aBreak assertion fix should be actually in previous patch, about
nsRenderingContextBeOS, but we forgot it - i was sure for unknown reason that we
fixed this long ago:)

But i submitted new patch in "Japanese" bug, which fixes this issue too. You was
CC-ed.
confirmed aBreaks issue fixed with Japanese monospace font patch, as fyysik
indicates.  I've been running with tqh's latest patch for this for about 2 days,
with no apparent problems.  May be good for review, tqh.
we might want to make the printf("NO PARENT!!!\n") warning only showup on debug
builds.  I can't begin to guess why the code falls through to this point, but it
doesn't seem to hurt the operation of the browser.  I've only seen it at
startup, as noted above.  
That is the plan, but before I submit the final one I wanted one that shows that
there actually is such a case. Before we commit it I want to find out why as well.
Attached patch mWindowType mBorderStyle changes (obsolete) — Splinter Review
This one handles all windowtypes I could test correctly. That means that we
have Firefox 'Customize toolbar' without border. It's very rewritten, but with
some comments.
Attachment #197461 - Attachment is obsolete: true
Hmm, looking at Firefox in windows I see that
Prefs have close, title, and resize. No minimize, maximize.

And customize toolbar has:
title, minimize, maximize, close and resize.
I've applied the patch over clean nsWindow from CVS.  Hunk #9 (line 626) failed
to apply cleanly, for some reason, but was easily applied by hand since it
appears to be cleanup only and not new code.  No adverse effects yet and it sure
is nice to be able to resize the preferences window!
(In reply to comment #27)
> That is the plan, but before I submit the final one I wanted one that shows that
> there actually is such a case. Before we commit it I want to find out why as well.

Any luck finding out why we have windows withouth parents?
Havn't looked at that yet, I'm trying to see how window-styles should be.
No longer blocks: 266252
Blocks: 311032
have you made any further progress on this, tqh?  I'm about to do a bunch of
Firefox builds and would like to include the work-in-progress, if only because
it makes the options/preferences window much easier to use under BeOS.  
My motivation is at an all time low, it depends on factors which I have no
control over.
(In reply to comment #34)
> My motivation is at an all time low, it depends on factors which I have no
> control over.

tqh, I'm truly sorry to hear you're so bummed out.  The work you've done so far
is already a huge improvement over previous window behavior.  Please let me know
if there's anything at all I can do to help you feel better. 
Well, it only lasted a short while :) Apparently I was mistaken about some
things. Still my laptop is having wifi problems so I have to work on other things.
(In reply to comment #36)
> Well, it only lasted a short while :)
I'm happy for you!  but bummer about your laptop.  Anything I can do to help in
the meantime?


Attached patch Updated to work with current CVS (obsolete) — Splinter Review
Changes:
GetNativeData changed so needed to call with other type.
SetBounds is called so that redraw is done correctly.
IME is added, but not sure it will work.
mParent is set.

Reordered variables (so that they might align better) or if we want
to change to PRPackedBool.
Attachment #197752 - Attachment is obsolete: true
Looks ok, testing.
Though, probably
printf("UNKNOWN or not handled windowtype!!!\n");
should be put in assertion or ifdef debug in future
//NS_NATIVE_WIDGET maybe?
//Parent may be a BView if we embed.
BView *parent= (BView *) (aParent ? aParent->GetNativeData(NS_NATIVE_GRAPHIC) : aNativeParent);

We may change GetNativeData() method back if needed. It might be also NS_NATIVE_WIDGET here.
1) I don't see break or return after
case eWindowType_plugin:

Is NS_NOTYETIMPLEMENTED("*") macro for that? Or it only printouts?

2) I'm unsure if toplevel BView/nsViewBeOS needs B_FRAME_EVENTS flags.
Only native resize event we're getting now is from BWindow::FrameResized()
hmm, i got some strange feeling that invisible window is working as coordinate origin/parent, common for windows, which are in some sence in parent/sibling relations, like main window and (preferences) dialog. So, mouse coordinates and maybe some other events use its bounds as origin to calculate relative coordinates.

I will try to investigate it more, but maybe we will need to reallow BaseCreate and create event dispatching for it again.
case eWindowType_plugin fallbacks to childview (and should be tested with plugins), it just prints out a warning before it does. 
We need one more module-level variable to be initialized in Create:
mListenForResizes = aInitData->mListenForResizes;
to use it then in ::Resize() method:
	if (mListenForResizes || mIsTopWidgetWindow)
		OnResize(mBounds);

in order to avoid unneeded message sending
I think it's time to close this one if the patch came out alright. r?
Attachment #202032 - Attachment is obsolete: true
Attachment #205170 - Flags: review?(sergei_d)
Comment on attachment 205170 [details] [diff] [review]
Some cleanup and suggested changes

there was an error in one of the default flags. Corrected patch on its way.
Attachment #205170 - Attachment is obsolete: true
Attachment #205170 - Flags: review?(sergei_d)
r?
Attachment #205173 - Flags: review?(sergei_d)
Comment on attachment 205173 [details] [diff] [review]
Removed B_NO_WORKSPACE_ACTIVATION for default toplevel windows

r=sergei_d

actually i dislike
BRect aViewRect...

aFoo is for argument.
No need for such style for variable which you will put in function call, it is confusing, as in this case, for illustration, we should look for this among StandardWindowCreate() parameters.

Actually, maybe we don't need to create there this temp variable at all, but use conversion from screen inside new nsViewBeOS() call instead.
Attachment #205173 - Flags: review?(sergei_d) → review+
excellent!  I just built with the version 3 patch and my nasty bug with session history max viewers (fastback bug 301894) seems to be gone!  I could not duplicate the problem using any of the sites/techniques that caused trouble before.
Doug, I think you linked the wrong bug :)

Sergei, I agree. It shouldn't be needed.
s/301834/ bug 308194
Sergei your comment made me look into that. I think I have some changes:

			mView = new nsViewBeOS(this, w->Bounds(), "Toplevel view", 0, (mWindowType == eWindowType_popup ? B_WILL_DRAW: 0)  | B_FRAME_EVENTS);

and removing the mView->ResizeTo line and the line above.

Think we need to discuss this, seems to work. B_FOLLOW_ALL is removed btw.
Oops, it seems that I may have been fooled and it used an older lib.

It might need B_FOLLOW_ALL after all.
for toplevel we DO need FOLLOW_ALL but DON'T need (with current implementation) FRAME_EVENTS.
Maybe you confused with those two.
r?
This one I think should be checked in.
Attachment #205316 - Flags: review?(sergei_d)
I have incorporated patch 4 into my build.  The problems with fastback cache have come back.  I'll switch it back and rebuild to make sure the only change is patch 3 compared to patch 4.
No explanation: I went back and rebuilt with 3 and now I'm reproducing the problem again.  I can't explain - I doublechecked settings, etc.  I did a clean build with the version 3 patch.  I apologize for the bad information.
Comment on attachment 205316 [details] [diff] [review]
changed some lines at nsWindowBeOS creation

r=sergei_d
Attachment #205316 - Flags: review?(sergei_d) → review+
Comment on attachment 205173 [details] [diff] [review]
Removed B_NO_WORKSPACE_ACTIVATION for default toplevel windows

obsoleting
Attachment #205173 - Attachment is obsolete: true
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.113; previous revision: 1.112
done
Checking in mozilla/widget/src/beos/nsWindow.h;
/cvsroot/mozilla/widget/src/beos/nsWindow.h,v  <--  nsWindow.h
new revision: 1.48; previous revision: 1.47
done 

landed in trunk.
Wating for first build from cvs to close bug.
sergei, I think you can close the bug.  I deleted widget/src/beos directory and checked out a new one from CVS, then did a make -f client.mk clean build.  No compile errors, clean build.  I'm using it now.
closing
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Requesting permission to land on the MOZILLA_1_8_BRANCH.

This is a BeOS-only change and does not affect other platforms.
Comment on attachment 233136 [details] [diff] [review]
Patch for 1_8 branch

BeOS only change
Attachment #233136 - Flags: approval1.8.1?
Comment on attachment 233136 [details] [diff] [review]
Patch for 1_8 branch

a=beltzner on behalf of drivers for the 1.8.1 branch (NPOTB)
Attachment #233136 - Flags: approval1.8.1? → approval1.8.1+
Attachment #195957 - Flags: review?(sergei_d)
I'm confused.  I received an update to this bug earlier today but now cannot see the attachment represented by this activity:
steffen.wilberg@web.de  	2007-05-31 11:03:07 PDT  	  Attachment #195957 [details] [diff]Flag  	review?(sergei_d@fi.tartu.ee)  	 

It appears Steffen didn't realize the patch was committed and tried to introduce additional changes.  Was a new bug created with those additional changes?
All Steffen did, which you can see by viewing https://bugzilla.mozilla.org/show_activity.cgi?id=296856, was remove an old review request.
(In reply to comment #67)
> All Steffen did, which you can see by viewing
> https://bugzilla.mozilla.org/show_activity.cgi?id=296856, was remove an old
> review request.
> 
Ah, got it now.  Thank you Gavin!
If I'm reading the code correctly, the patch for the 1.8 branch does not appear to have been committed.  Neilx, if you're reading this can you please try to verify?
It does not have the appropriate keywords, and I have not found any commits in bonsai between those dates, so I'm pretty sure this one wasn't applied.
Blocks: 383065
sergei, it looks like this is another one to apply to branch.  It was approved but maybe never actually placed in the branch code.
(In reply to comment #71)
> sergei, it looks like this is another one to apply to branch.  It was approved
> but maybe never actually placed in the branch code.
> 
If it still applies and works without side-effects, and this approval is still valid, I will check that in
(In reply to comment #71)
> sergei, it looks like this is another one to apply to branch.  It was approved
> but maybe never actually placed in the branch code.
> 
Floating/Modal code is already in branch.
(In reply to comment #73)
> (In reply to comment #71)
> > sergei, it looks like this is another one to apply to branch.  It was approved
> > but maybe never actually placed in the branch code.
Windows creation patch does not apply here for branch.

Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.