[BeOS] widget code cleanup

RESOLVED FIXED

Status

Core Graveyard
Widget: BeOS
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Sergei Dolgov, Assigned: Sergei Dolgov)

Tracking

({fixed1.8.1})

Trunk
x86
BeOS
fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

13 years ago
there are several methods with no use in our code:
Set/Get Foregound,
AutoErase(),
EnableFileDrop().
There are several unused #include statements.
Also, some variable names may be more reasonable.
(Assignee)

Comment 1

13 years ago
No more need for DoDraw - as we don't use DrawOnChildren hook anymore.
So all code can be safely moved to nsViewBeIS::Draw(), i think
(Assignee)

Comment 2

13 years ago
tqh proposes also GetBounds() as candidate to be removed...i think it needs more investigation

Comment 3

13 years ago
It might need a bug of it's own.

Comment 4

13 years ago
AutoErase I think is used but I don't know how that works.

Comment 5

13 years ago
Created attachment 201941 [details] [diff] [review]
Example of what to remove
(Assignee)

Comment 6

13 years ago
1)Maybe we keep GetBounds() as reminderm, just returning mBounds?
2) Are you sure you can call mView->Frame() without locking looper? We can do it for BWindow->Frame(), but i doubt about BView::Frame()
3) There is another candidate to eliminate, besides obvious DoDraw and AutoErase - OnScroll(). IIRC it was used (or there was attempt to use it) only when buggy native scrollbar code existed in our tree.
4)As far as i recall, it is much more convinient for us to use bool OnPaint() instead PRBool. Will look again at that, in attempt to recall where i got this opinion from:)

Will test GetBounds() elimination next days and try to figure something more to cleanup:)
(Assignee)

Comment 7

13 years ago
next candidate - GetHeight() - also some very specific MS Windows-only thing, as AutoErase(), heritage of very intial porting attempt.

Btw, with all those rect transitions i have big temptation to add at some moment two inlined functions, in widget, but as weel in gfx:
CovertFromBRect()
and 
ConvertToBRect()

Comment 8

13 years ago
Sounds like a good idea.

Comment 9

13 years ago
I added locking and null-check in ONRESIZE for safety. Although I had no issues without it.

Comment 10

13 years ago
Created attachment 201983 [details] [diff] [review]
Example of what to remove 2

Removed some unused variables, and duplicate initialization of BaseWidget variables.
Attachment #201941 - Attachment is obsolete: true
(Assignee)

Comment 11

13 years ago
I tried to post here comment this night, but maybe i'be put it in another bug, mistakenly. Cannot find now.
It was about removing SetBackgroundColor(). After testing I'm against removing it.
At least at my slower machine it produces ugly visual appearance, whith (fast) resize to larger size. 
Though, didn't notice any issue with removing SetForegroundColor.

Next thing i'm wondering about is Set/GetFont. Looks like it is also intended for case when we use some native widgets, but we don't. But as in BaseWidget those are pure virtuals, it is hard to remove it from our code. Will investigate things bit more.
(Assignee)

Comment 12

13 years ago
Tqh, actually i'm wondering which version of BeOS you use?
According to BeBook, BView::Frame() cannot be called without crash in non-loced state.
At this is absolutely true for my BeOS at least. So it crashes instantly at standard window create at mView->Frame() with very standard BeOS debug message:
"Looper must be locked".
This is that sad reason why i forced to invent tricks in gfx, when we draw even at BBitmap, not BWindow, and when we use single thread with guarantee - all those
 BView method simply do crash.

Comment 13

13 years ago
For some reason it didn't in the static build I had.
(Assignee)

Comment 14

13 years ago
next cadidate to remove:
nsViewBeOS::AttachedToWindow() - by moving SetViewColor to nsViewBeOS constructor.

also, SetBackgroundColor will remain, but with check for mIsTopWidgetWindow.

Set/GetFont - no need to set BView font here. Copy code from mac, and add nsRenderingContext::SetFont() in OnPaint().  It will allow remove ugly hack with be_plain_font in nsRenderingContextBeOS.

Comment 15

13 years ago
Created attachment 201987 [details] [diff] [review]
Example of what to remove 3
Attachment #201983 - Attachment is obsolete: true
(Assignee)

Comment 16

13 years ago
Created attachment 201990 [details] [diff] [review]
patch

additions:
ONSCROLL and OnScroll() removed.
also MENU and CREATE_HACK removed from enum.
DoDraw() removed.
Get/SetFont() code refactored, added notice about SetFont in ::OnPaint().
OnPaint() return type changed to nsresult for more convinience (we used it wrong in Invalidate() methods).
Code around mView->Frame() refactored a bit.
restoreMouseMask name changed to be more clear that it is flag.

Also i don't see any rational reason to change style of variable initialization in constructors. So made it more consistent.
also moved two variable declarations in nsViewBeOS to be together with other public items.

Now we need to decide about removing from the folder following items:
nsButton.*
nsCheckButton.*
nsTextHelper.*
nsLabel.*
nsTextHelper.*
nsTextWidget.*

Also I have question about role of nsObject.*
Attachment #201987 - Attachment is obsolete: true
(Assignee)

Comment 17

13 years ago
Created attachment 201992 [details] [diff] [review]
patch diff -up8

review request
Attachment #201992 - Flags: review?(thesuckiestemail)

Comment 18

13 years ago
Comment on attachment 201992 [details] [diff] [review]
patch diff -up8

r=thesuckiestemail@yahoo.se

mIsDestroying, mOnDestroyCalled,
mWindowType,
mBorderStyle
 is already set in BaseWidget(). So we can remove that too. But we can add that in new patch.
Attachment #201992 - Flags: review?(thesuckiestemail) → review+
(Assignee)

Comment 19

13 years ago
I think those 3 is for StandardWindowCreate patch:)
I'm for removing it there, especially if you add PreCreate code with setting BorderStyle there.
(Assignee)

Comment 20

13 years ago
patch
https://bugzilla.mozilla.org/attachment.cgi?id=201992
checked in trunk
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.107; previous revision: 1.106
done
Checking in mozilla/widget/src/beos/nsWindow.h;
/cvsroot/mozilla/widget/src/beos/nsWindow.h,v  <--  nsWindow.h
new revision: 1.43; previous revision: 1.42
done

Time to get rid of nsObject 

Comment 21

13 years ago
Yes, I think I should do a final version of stdwindowcreate soon.
(Assignee)

Comment 22

13 years ago
Created attachment 201994 [details] [diff] [review]
patch diff -up8. Removing nsObject.

review request.

nsObject is used only in BeOS and looks like legacy heritage of very early attempt to create BeOS port
Attachment #201990 - Attachment is obsolete: true
Attachment #201994 - Flags: review?

Comment 23

13 years ago
Comment on attachment 201994 [details] [diff] [review]
patch diff -up8. Removing nsObject.

r=thesuckiestemail@yahoo.se
Attachment #201994 - Flags: review? → review+
(Assignee)

Comment 24

13 years ago
patch
https://bugzilla.mozilla.org/attachment.cgi?id=201994
commited into trunk:
Checking in mozilla/widget/src/beos/nsWindow.h;
/cvsroot/mozilla/widget/src/beos/nsWindow.h,v  <--  nsWindow.h
new revision: 1.44; previous revision: 1.43
done
Checking in mozilla/widget/src/beos/nsAppShell.h;
/cvsroot/mozilla/widget/src/beos/nsAppShell.h,v  <--  nsAppShell.h
new revision: 1.11; previous revision: 1.10
done

Time to remove unused files and close bug at next step
Checking in mozilla/widget/src/beos/Makefile.in;
/cvsroot/mozilla/widget/src/beos/Makefile.in,v  <--  Makefile.in
new revision: 1.50; previous revision: 1.49
done 
Assignee: nobody → sergei_d
(Assignee)

Comment 25

13 years ago
extra files removed from trunk:
Removing nsButton.cpp;
/cvsroot/mozilla/widget/src/beos/nsButton.cpp,v  <--  nsButton.cpp
new revision: delete; previous revision: 1.13
done
Removing nsButton.h;
/cvsroot/mozilla/widget/src/beos/nsButton.h,v  <--  nsButton.h
new revision: delete; previous revision: 1.5
done
Removing nsCheckButton.cpp;
/cvsroot/mozilla/widget/src/beos/nsCheckButton.cpp,v  <--  nsCheckButton.cpp
new revision: delete; previous revision: 1.10
done
Removing nsCheckButton.h;
/cvsroot/mozilla/widget/src/beos/nsCheckButton.h,v  <--  nsCheckButton.h
new revision: delete; previous revision: 1.5
done
Removing nsLabel.cpp;
/cvsroot/mozilla/widget/src/beos/nsLabel.cpp,v  <--  nsLabel.cpp
new revision: delete; previous revision: 1.12
done
Removing nsLabel.h;
/cvsroot/mozilla/widget/src/beos/nsLabel.h,v  <--  nsLabel.h
new revision: delete; previous revision: 1.5
done
Removing nsObject.cpp;
/cvsroot/mozilla/widget/src/beos/nsObject.cpp,v  <--  nsObject.cpp
new revision: delete; previous revision: 1.4
done
Removing nsObject.h;
/cvsroot/mozilla/widget/src/beos/nsObject.h,v  <--  nsObject.h
new revision: delete; previous revision: 1.4
done
Removing nsTextHelper.cpp;
/cvsroot/mozilla/widget/src/beos/nsTextHelper.cpp,v  <--  nsTextHelper.cpp
new revision: delete; previous revision: 1.13
done
Removing nsTextHelper.h;
/cvsroot/mozilla/widget/src/beos/nsTextHelper.h,v  <--  nsTextHelper.h
new revision: delete; previous revision: 1.6
done
Removing nsTextWidget.cpp;
/cvsroot/mozilla/widget/src/beos/nsTextWidget.cpp,v  <--  nsTextWidget.cpp
new revision: delete; previous revision: 1.16
done
Removing nsTextWidget.h;
/cvsroot/mozilla/widget/src/beos/nsTextWidget.h,v  <--  nsTextWidget.h
new revision: delete; previous revision: 1.5
done
----------
Closing bug
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 26

12 years ago
Will be backported to the 1_8 branch.
Blocks: 311032

Comment 27

12 years ago
Created attachment 230875 [details] [diff] [review]
Patch for 1_8 branch

Combines previous patches and makes sure they apply cleanly.

No changes.

Requesting approval. This is a BeOS-only change and does not affect any other platforms. Also requesting approval for removing unused files:
- nsButton.*
- nsCheckbutton.*
- nsLabel.*
- nsObject.*
- nsTextHelper.*
- nsTextWidget.*
Attachment #230875 - Flags: approval1.8.1?

Updated

12 years ago
Attachment #230875 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 28

12 years ago
Checking in mozilla/widget/src/beos/Makefile.in;
/cvsroot/mozilla/widget/src/beos/Makefile.in,v  <--  Makefile.in
new revision: 1.47.14.2; previous revision: 1.47.14.1
done
Checking in mozilla/widget/src/beos/nsAppShell.h;
/cvsroot/mozilla/widget/src/beos/nsAppShell.h,v  <--  nsAppShell.h
new revision: 1.10.28.1; previous revision: 1.10
done
Removing mozilla/widget/src/beos/nsButton.cpp;
/cvsroot/mozilla/widget/src/beos/Attic/nsButton.cpp,v  <--  nsButton.cpp
new revision: delete; previous revision: 1.13
done
Removing mozilla/widget/src/beos/nsButton.h;
/cvsroot/mozilla/widget/src/beos/Attic/nsButton.h,v  <--  nsButton.h
new revision: delete; previous revision: 1.5
done
Removing mozilla/widget/src/beos/nsCheckButton.cpp;
/cvsroot/mozilla/widget/src/beos/Attic/nsCheckButton.cpp,v  <--  nsCheckButton.cpp
new revision: delete; previous revision: 1.10
done
Removing mozilla/widget/src/beos/nsCheckButton.h;
/cvsroot/mozilla/widget/src/beos/Attic/nsCheckButton.h,v  <--  nsCheckButton.h
new revision: delete; previous revision: 1.5
done
Removing mozilla/widget/src/beos/nsLabel.cpp;
/cvsroot/mozilla/widget/src/beos/Attic/nsLabel.cpp,v  <--  nsLabel.cpp
new revision: delete; previous revision: 1.12
done
Removing mozilla/widget/src/beos/nsLabel.h;
/cvsroot/mozilla/widget/src/beos/Attic/nsLabel.h,v  <--  nsLabel.h
new revision: delete; previous revision: 1.5
done
Removing mozilla/widget/src/beos/nsObject.cpp;
/cvsroot/mozilla/widget/src/beos/Attic/nsObject.cpp,v  <--  nsObject.cpp
new revision: delete; previous revision: 1.4
done
Removing mozilla/widget/src/beos/nsObject.h;
/cvsroot/mozilla/widget/src/beos/Attic/nsObject.h,v  <--  nsObject.h
new revision: delete; previous revision: 1.4
done
Removing mozilla/widget/src/beos/nsTextHelper.cpp;
/cvsroot/mozilla/widget/src/beos/Attic/nsTextHelper.cpp,v  <--  nsTextHelper.cpp
new revision: delete; previous revision: 1.13
done
Removing mozilla/widget/src/beos/nsTextHelper.h;
/cvsroot/mozilla/widget/src/beos/Attic/nsTextHelper.h,v  <--  nsTextHelper.h
new revision: delete; previous revision: 1.6
done
Removing mozilla/widget/src/beos/nsTextWidget.cpp;
/cvsroot/mozilla/widget/src/beos/Attic/nsTextWidget.cpp,v  <--  nsTextWidget.cpp
new revision: delete; previous revision: 1.16
done
Removing mozilla/widget/src/beos/nsTextWidget.h;
/cvsroot/mozilla/widget/src/beos/Attic/nsTextWidget.h,v  <--  nsTextWidget.h
new revision: delete; previous revision: 1.5
done
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.91.4.15; previous revision: 1.91.4.14
done
Checking in mozilla/widget/src/beos/nsWindow.h;
/cvsroot/mozilla/widget/src/beos/nsWindow.h,v  <--  nsWindow.h
new revision: 1.35.4.10; previous revision: 1.35.4.9
done 
Keywords: fixed1.8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.