[motif] Improved layout for wxChoice and wxButton

wxtrac commented 20 years ago

Issue migrated from trac ticket # 6330

component: wxMotif | priority: normal

2003-11-04 21:57:10: justinbb created the issue

Many of the layout problems in the Motif implementation have been fixed but not yet released. Even with the new changes, choice widgets are not sized quite right and button widgets do not align correctly with choice widgets.

This patch applies to wxWindows 2.4.2. It does two things:

provide the layout fixes that are in CVS but not yet released
improve the sizing of choice and button widgets (these changes should also apply to the trunk).

I will provide separate patches for the trunk on request.

wxtrac commented 20 years ago

2003-11-10 11:24:56: ibrown commented

How do I go about applying this patch? I tried the normal:

patch -i patchfile

from the wxWindows directory, but all the hunks failed. Did you make the patch in some special way? Do I need some special parameters to pass to patch?

Normally, to make a patch, I do cvs diff -bu2 > patch.diff from the wxWindows directory.

It's great to see yet another person working with motif...

wxtrac commented 20 years ago

2003-11-10 14:21:38: ibrown commented

I managed to apply it ... I had to add Index: filename

at the start of every new file in you patchfile. I have a cvs diff of your patch now ... I can mail it to you if you have problems doing it yourself. Anything to make Mattia's life easier when patching :)

wxtrac commented 20 years ago

2003-11-10 15:34:07: justinbb commented

The simple way to apply the patch is to use "patch -p0" (because it contains significant pathname components, "src/motif" or "include/ wx/motif"). That's easier and more reliable than requiring Index: lines, I think!

In making the changes contained in this patch, I didn't try to solve the layout problem completely for all types of widgets. There are still some arbitrary constants in some widgets, and it's possible that I neglected to take some resource values into account. In other words, this is a starting point.

wxtrac commented 20 years ago

2003-11-10 15:37:00: ibrown commented

You need to additionally implement wxComboBox::DoGetSize(intx, int y) { wxWindow::DoGetSize(x, y); }

The patch as it stands does not do this, and so wxChoice::DoGetSize(int x, int y) is called. This is very bad because Xt functions are called with NULL widgets and this causes a crash.

I also noticed 2 small issues which I think this patch introduces...

The drop down list of the wxChoice is a few pixels too small horizontally for the widest text in it.
The 'OK' button in by about box is now to small vertically.

Ian

wxtrac commented 20 years ago

2003-11-10 15:40:38: ibrown commented

I tried -p0 and it didn't work on Solaris 9 (maybe it is a solaris issue). The problem is that patch only sees one file ... so it tries to apply all of the patches to the first file in the patch (bmpbutton.cpp). I think solaris patch wants *** in front of the name rather than +++. Either way, it's happy with the format that cvs - bu2 spits out ... so if it is possible to make patches that way in the future that would be great (at least for me ;-)

wxtrac commented 20 years ago

2003-11-10 17:14:57: justinbb commented

Ah, it looks like Solaris 9's patch command doesn't understand unified diffs (diff -u) unless you specify the -u option to patch. Without that, your patch command is expecting output from "diff -c" but because of patch's advanced fuzzy-matching logic, it manages to work around the format difference if the Index: lines produced by cvs are present.

I didn't make my diffs from CVS because my base was the gzipped tar archive of the 2.4.2 distribution.

Try "patch -u -p0". Note that you should be using -u for all diffs produced with diff -u, not just mine (or install Gnu patch on your system).

Meanwhile, I'll fetch wxWindows 2.4.2 from CVS so I can make future patches with CVS, including the Index: line.

justin

wxtrac commented 20 years ago

2003-11-10 17:20:19: ibrown commented

That sounds right ... it said it detected a context diff. Thanks for the tip!

wxtrac commented 20 years ago

2003-11-11 00:11:04: justinbb commented

Here's a new patch to tidy up a bit:

made with CVS diff, so it includes Index: lines
protect against NULL m_buttonWidget in wxChoice::DoGetSize, so derived classes that don't use m_buttonWIdget are not in danger of crashing

In reply to Ian's other two issues: (1) [choice widget width] I am not seeing this problem (widget not quite wide enough) except in one case: if wxwindows requests a width that is big enough for the second-longest menu item, this width will be granted, even if the longest item gets trimmed. If the requested width is smaller than the second-longest item, the control snaps back to full width and messes up the layout. I have no idea where this behavior comes from (perhaps OpenMotif's motif/lib/Xm/RCLayout.c, but I am not courageous enough to figure that file out today).

(2) [button widget height] How is "too small vertically" to be judged? I reduced the arbitrary minimum height rather drastically, so that Motif could choose the height depending on the font size. There are Motif resources to request margins on buttons, and there are wxWindows parameters to request a particular size. But the question is open to debate. The reason for my choice was that I needed button and choice widgets to have the same height, and I wanted this height to depend on the size of the font being used.

justin

2003-11-11 00:11:04: justinbb uploaded file `wxWindows-2_4-motif.patch` (20.0 KiB)

Patch for Motif widget layout in 2.4.2

wxtrac commented 20 years ago

2003-11-11 11:55:33: ibrown commented

(1) now that I look at it, it is perfectly sized for the second widest item. The widest item is the last in the list. The choice is in a toolbar. I'll look into it a bit more .. the strings are static so in the worst case I can just set a fixed static width for the control.

(2) It's an OK button and only the mid half of the characters are displayed .. i.e. it needs to be twice the size just to fit the characters. It's created like this...

m_ok = new wxButton( this, wxID_OK, "OK" ); button_sizer->Add( m_ok, 0, // make horizontally unstretchable wxALL, // make border all around (implicit top alignment) 10 ); // set border width to 10

I'll have a play with the sizer settings / border widths and see if I can get something that works with the patch. All the buttons on my other dilaogs still look okay, it's just this one that looks bad with the patch.

wxtrac commented 20 years ago

2003-11-11 13:19:29: ibrown commented

The new patch applies fine on solaris.

I have tracked down the button issue ... it is because it is a default button. If I remove the call to set it as default: m_ok->SetDefault(); Then it is okay. It seems that the code that caculates the required size of the button does not take into account the extra border motif gives default buttons.

wxtrac commented 20 years ago

2003-11-11 13:37:02: ibrown commented

You can see the button size problem by running the dialogs sample. From the File menu choose Modal Dialog and you will see the problem when sizing a default button. There is also an issue with the text control size .... I'm not sure but I think the patch may have introduced this too. I know that the problem is new in 2.5 and I don't recall seeing it in an unpatched 2.4 before today. When you click a button on the modal dialog in the dialogs sample, another dialog pops up which demonstrates the issue with the text control sizing.

wxtrac commented 20 years ago

2003-11-11 15:42:25: justinbb commented

You're right, I didn't take into account the default highlight. Thanks for reminding me of the dialogs sample; it's a much more thorough test than my own app. I'll take a look at these problems soon.

wxtrac commented 20 years ago

2003-11-12 11:11:03: ibrown commented

Just checked and the static text sizing is nothing to do with this patch .. it also exists in plain 2.4.2. So, once the size of default buttons is fixed I think we're good to go.

wxtrac commented 20 years ago

2003-11-12 12:16:24: ibrown commented

Found and fixed the dialog text issue. Fix is under patch number 840643. It has nothing to do with this patch.

wxtrac commented 20 years ago

2003-11-13 19:49:34: justinbb commented

The default button size problem is not easy. The code that's in my patch that calculates the size of a button was taken from 2.5. What I did that makes the issue stand out was to reduce the default button size to something very small. The workaround in 2.5 is to make the default button size very big.

The fundamental problem is that in Motif, designating a button the default increases its size drastically, and this tends to happen after the button size has been fixed in wxWindows. Is it possible to require default-button designation at create time? Or to have an option at create-time to request that buttons add in the extra size for the default indicator?

Other things that need to be dealt with: the size of the default indicator is variable, so it should probably be set as small as possible. And the button-sizing code from 2.5 is not quite right, since it doesn't take into account enough resources (Motif is SO complex, argh). I can take care of these two minor issues - though I won't have a chance to work on it until next week.

In the meantime, perhaps the best quick fix for now would be to increase the default size of the button; that won't be any worse than what 2.4.2 or 2.5 currently do. The Right Solution for button sizing can then be deferred.

wxtrac commented 20 years ago

2003-11-13 19:54:04: justinbb commented

Ooops, confusion in my previous text between "size of a default button" and "default size for a button".

It's GetDefaultSize() that I modified to return small values, that should probably be put back to something large as a workaround for this problem.

Elsewhere I'm talking about the button with a big ring around it!

wxtrac commented 20 years ago

2004-02-07 15:44:53: @vadz commented

The patch doesn't apply [at all] against cvs HEAD, is there any chance of producing a patch for inclusion in the upcoming 2.5.1 release?

Thanks!

wxtrac commented 20 years ago

2004-02-16 21:39:34: justinbb commented

Sorry for the slow reply. I hope to be able to spend some time on wxWindows shortly and will produce a new patch.

wxtrac commented 19 years ago

2004-07-25 13:21:12: @vadz commented

The patch can never be applied in its current state, unfortunately, so unless we can have something which could be applied to 2.5.2 it is not useful to keep it here.

wxWidgets / wxWidgets