esternin / eXtrema

https://www.physics.brocku.ca/Labs/extrema/
GNU General Public License v2.0
6 stars 1 forks source link

GUI: secondary/pop-up windows are of insufficient size #3

Closed esternin closed 3 years ago

esternin commented 3 years ago

Pretty much all windows (e.g. Analysis Window -> File -> Execute) pop up in the size too small to fit all content (text, inputs, buttons) within them.

esternin commented 3 years ago

Perhaps a variation of this problem, perhaps a different one. The inquire pop-up is not resizable, so increasing its size with a mouse to reveal the buttons is not possible. This is called multiple times by Scripts/demo.pcm. This style of pop-up is supposed to contain yes/no buttons, and it has nothing; to go on, one can kill (x in upper right) the pop-up and the script continues.

vadz commented 3 years ago

Weird, I don't see this at all here. Or, rather, the dialog is indeed not resizeable (I'll check if it has the correct styles), but it does have the right size by default. Which desktop environment/WM do you use?

Also, is this with GTK 2 or 3?

esternin commented 3 years ago

Xubuntu 20.04 LTS, up-to-date, xfce, have no trace of gtk2 on the system.

This is as it pops up: image

After a resize: image

After the first inquire pops up, no way to proceed, no buttons, inquire dialog not resizeable, but can kill it and the script goes on: image

esternin commented 3 years ago

on the inquire pop-up: press TAB once, then return, and the pop up closes. So the "OK" or "yes" button is there, just off widget, and the widget cannot resize to show it.

vadz commented 3 years ago

The fact that widget doesn't resize is just due to not using the appropriate style and this would fix it:

diff --git a/src/wxForms/InquireDialog.cpp b/src/wxForms/InquireDialog.cpp
index 7b75a35..6e033df 100644
--- a/src/wxForms/InquireDialog.cpp
+++ b/src/wxForms/InquireDialog.cpp
@@ -35,7 +35,8 @@
 END_EVENT_TABLE()

 InquireDialog::InquireDialog()
-    : wxDialog( (wxWindow*)NULL,wxID_ANY,wxT("inquire"),wxDefaultPosition,wxDefaultSize )
+    : wxDialog( (wxWindow*)NULL,wxID_ANY,wxT("inquire"),wxDefaultPosition,wxDefaultSize,
+                wxDEFAULT_DIALOG_STYLE | wxRESIZE_BORDER )
 {
   wxBoxSizer *mainSizer = new wxBoxSizer( wxVERTICAL );

but the real issue is, of course, that the window title bar height is not taken into account here, this dialog doesn't actually need to be resizeable.

If you're happy with just making it resizeable, please let me know. Otherwise, I think the problem is with the code saving/restoring the dialog geometry, e.g. it hardcodes the height of this dialog as 145 which just seems insufficient for your system. The best thing to do would be to get rid of this code and use built-in support for saving/restoring the dialog position and size. Please assign this one to me if you'd like me to do it.

esternin commented 3 years ago

Yes, and the same fix will be needed in a lot of dialogs.

esternin commented 3 years ago

This is what shows on the console during the execution of demo.pcm, as I hit Tab-Return to move to and press the first invisible button ("Yes", of Yes/No/Stop all scripts) in case yours does not:

...
(extrema:295025): Gtk-WARNING **: 11:00:51.508: Negative content height -3 (allocation 1, extents 2x2) while allocating gadget (node button, owner GtkButton)
(extrema:295025): Gtk-WARNING **: 11:00:51.508: Negative content height -3 (allocation 1, extents 2x2) while allocating gadget (node button, owner GtkButton)
(extrema:295025): Gtk-WARNING **: 11:00:51.508: Negative content height -3 (allocation 1, extents 2x2) while allocating gadget (node button, owner GtkButton)
(extrema:295025): Gtk-WARNING **: 11:00:51.916: Negative content height -3 (allocation 1, extents 2x2) while allocating gadget (node button, owner GtkButton)
(extrema:295025): Gtk-WARNING **: 11:00:51.916: Negative content height -3 (allocation 1, extents 2x2) while allocating gadget (node button, owner GtkButton)
(extrema:295025): Gtk-WARNING **: 11:00:51.916: Negative content height -3 (allocation 1, extents 2x2) while allocating gadget (node button, owner GtkButton)
(extrema:295025): Gtk-WARNING **: 11:00:54.130: Negative content height -3 (allocation 1, extents 2x2) while allocating gadget (node button, owner GtkButton)
(extrema:295025): Gtk-WARNING **: 11:00:54.149: Negative content height -3 (allocation 1, extents 2x2) while allocating gadget (node button, owner GtkButton)
(extrema:295025): Gtk-WARNING **: 11:00:54.164: Negative content height -3 (allocation 1, extents 2x2) while allocating gadget (node button, owner GtkButton)
(extrema:295025): GLib-GObject-WARNING **: 11:00:54.226: ../../../gobject/gsignal.c:2736: instance '0x55817a4cccf0' has no handler with id '30726'
(extrema:295025): GLib-GObject-WARNING **: 11:00:54.228: ../../../gobject/gsignal.c:2736: instance '0x55817a4ccdd0' has no handler with id '30899'
(extrema:295025): GLib-GObject-WARNING **: 11:00:54.229: ../../../gobject/gsignal.c:2736: instance '0x55817a563010' has no handler with id '30549'
...

If, on the other hand, I hit three Tabs and then Return (to get to "Stop all scripts"), I get image

and continuing produces a segfault:

(extrema:295025): GLib-GObject-WARNING **: 11:08:45.082: ../../../gobject/gsignal.c:2736: instance '0x55817a29e300' has no handler with id '31778'
(extrema:295025): GLib-GObject-WARNING **: 11:08:45.084: ../../../gobject/gsignal.c:2736: instance '0x55817a4cceb0' has no handler with id '31951'
(extrema:295025): GLib-GObject-WARNING **: 11:08:45.086: ../../../gobject/gsignal.c:2736: instance '0x55817a4ccb30' has no handler with id '31601'
../src/common/strconv.cpp(1188): assert "Assert failure" failed in FromWChar(): trying to encode undefined Unicode character
(extrema:295025): Gtk-WARNING **: 11:08:45.223: gtk_window_set_titlebar() called on a realized window
../src/common/strconv.cpp(1188): assert "Assert failure" failed in FromWChar(): trying to encode undefined Unicode character
(extrema:295025): Gtk-WARNING **: 11:09:30.064: gtk_window_set_titlebar() called on a realized window
Segmentation fault
vadz commented 3 years ago

This seems like a completely different bug (or even a couple of different bugs), so I'd rather open new issues for it and keep this one just for the size problem, if you don't mind.

esternin commented 3 years ago

Size: I changed 145 -> 245 and it did not change the size of the Inquire pop-up. And adding

    wxDEFAULT_DIALOG_STYLE | wxRESIZE_BORDER )

did not make it resizable.

esternin commented 3 years ago

OK, the issue is an interaction with ~/.extrema which saves the state of the windows and widgets. Something about it not being updated on exit, if it exists. Even though the settings were changed in src/wxForms/InquireDialog.cpp, the ones in ~/.extrema remained at 145, and overrode that. I still do not understand why the resizeabilty could not be controlled.

vadz commented 3 years ago

I didn't have time to do it yet, but I'll remove all the code for storing/restoring the window sizes and just use built-in support for this instead, so you don't need to spend time on fixing this.

wxRESIZE_BORDER ought to work, I'll look at this too when I work on this.

esternin commented 3 years ago

Also, the parameters to [VisualizationWindow] are not being saved to ~/.extrema, but being properly read if entered there by hand.

I have to leave it in your capable hands :-)

Cheers!

vadz commented 3 years ago

This should be fixed by 88df269b67ef06d79d84a4e3b46f7a58da66174f, please close it if it works for you now.

If it still doesn't, for which window does the problem still exist and what do you have stored for it in your ~/.extrema file if it happens beyond the first run with the new version? TIA!

esternin commented 3 years ago

The problem is fixed in the new version, but...

  1. the very first time the default sizes are disorientingly small. For example, on Analysis Window the (empty initially) log display collapses into a line. The Execute pop-up is too narrow to accommodate even the file name it remembers from previous runs (like Scripts/demo.pcm). Could these be adjusted to be at least a certain size, in the absence of a saved configuration?
  2. on subsequent runs, the placement and size of windows is not saved, every time it now revert to the sizes set in the exit of the first run.

what file needs to be deleted to emulate the first run? should it not be ~/.config/extrema ?

vadz commented 3 years ago

There are 2 possibilities:

  1. Simple but not ideal is to use the previously hardcoded sizes by doing if ( !wxPersistentRegisterAndRestore(...) ) { SetSize(...); }, i.e. setting the fixed size if we didn't restore anything. This is not ideal because the hardcoded size will certainly be wrong on at least some systems (and possibly all...) anyhow, as it doesn't take into account DPI and font size, without even speaking about the differences between the UI element sizes between different platforms.
  2. A better way is to use wxSizer::SetMinSize() to make sure the elements that are too small have at least some size. To avoid the DPI/font size problems, the size should be computed using wxControl::GetSizeFromTextSize(largest-string-you-want-to fit) or approximated using GetCharWidth(), GetCharHeight() and/or GetTextExtent(), e.g. SetMinSize(wxSize(-1, 10*GetCharHeight())) would make it about 10 lines high. Like this the best size of the window will still be computed by the sizers and fit its contents.

This is still stored in ~/.extrema, we need to explicitly call wxStandardPaths::SetFileLayout(wxStandardPaths::FileLayout_XDG) to opt in into the new behaviour, but it's in 3.1 only.

esternin commented 3 years ago

I deleted the old ~/.extrema. I ran the newly compiled one, it opened all windows tiny. I resized, ran a demo.pcm, exited. Restarted, and the window sizes were as I set them on the first run.

But I have no ~/.extrema, not a trace of it. Not do I have any ~/.wx nor ~/.config/wx

PersistentManager stored the information somewhere else !?

On subsequent runs, all changes to the window sizes are NOT preserved, it always reverts back to the sizes set in that first run of the new version.

esternin commented 3 years ago

Never mind.

My apologies - I did not "make clear", so some .o was not recompiled. Now ~/.extrema is written out, is readable, and the program behaves reasonably.

The inquire\yesno dialog is still of a weird size/shape and the frame is not resizable, but no buttons are obscured.

The min sizes on the first run - in the absence of ~/.extrema - I would be happy with just setting them to 640x480 px for now. I may change my mind after testing on a high-DPI chromebook, but then we will need to do something about all button pixel images as well - they are just .bmp now.

vadz commented 3 years ago

My apologies - I did not "make clear", so some .o was not recompiled.

This does explain why I couldn't reproduce the problem, but it's still very strange, it's really supposed to rebuild properly whenever something is changed. I guess we won't spend time trying to reproduce this, but if you ever run into an apparently bad rebuild again, please let me know the details, as it must be some weird bug in the build system.

The inquire\yesno dialog is still of a weird size/shape and the frame is not resizable, but no buttons are obscured.

I didn't change the dialog to be resizeable, as I don't think it really needs it -- does it? The weird shape is just how it's laid out, I don't know why was it done like this, but it can be changed, of course.

The min sizes on the first run - in the absence of ~/.extrema - I would be happy with just setting them to 640x480 px for now.

Really all of them? If so, I'll add a helper function doing it and call it instead wxPersistentRegisterAndRestore everywhere (there are a couple of exceptions already doing something special, in HintForm and VisualizationWindow already). But I'm not sure if it's really going to be a good choice for all of them, they seem to have been using quite different default sizes before, doing git grep '/(WIDTH|HEIGHT)' before my changes I see 700x500, 320x435, 165x140, 570x145, 270x420 and more... Of course, this is what also resulted in truncation, but then 640x480 could still do it (e.g. it's smaller than the first one of those).

Personally I'd rather deal manually with the most egregiously bad examples but avoid hardcoding any sizes at all, this never works and they will have to be changed or removed later.

esternin commented 3 years ago

The inquire\yesno dialog is still of a weird size/shape and the frame is not resizable, but no buttons are obscured.

I didn't change the dialog to be resizeable, as I don't think it really needs it -- does it? The weird shape is just how it's laid out, I don't know why was it done like this, but it can be changed, of course.

Oh, I thought you were going to change the shape of the dialog to wrap around the widgets, whatever their size. If it's just static size, I can change it to something more reasonable.

The min sizes on the first run - in the absence of ~/.extrema - I would be happy with just setting them to 640x480 px for now.

Really all of them?

I meant only the Analysis and the Visualization windows. These are the first two that pop-up on startup, and they should be friendly. No graph should be too hard to seet, and no log/command window should be uninvitingly small.

This is just adding polish, and I can live with a need for the first-run resize. But I do think it would be better to accommodate this in the start-up: if ~/.extrema does not have settings (restore persistent fails), set these two windows to the 640x480 pixel and GetCharWidth(80)xGetCharHeight(20) minimum values, by doing a catch on wxPersistentRegisterAndRestore(this, "VisualizationWindow") in src/wxForms/VisualizationWindow.cpp and on wxPersistentRegisterAndRestore(this, "AnalysisWindow") in src/wxForms/AnalysisWindow.cpp.

esternin commented 3 years ago
 InquireDialog::InquireDialog()
-    : wxDialog( (wxWindow*)NULL,wxID_ANY,wxT("inquire"),wxDefaultPosition,wxDefaultSize )
+    : wxDialog( (wxWindow*)NULL,wxID_ANY,wxT("inquire"),wxDefaultPosition,wxDefaultSize,
+                wxDEFAULT_DIALOG_STYLE | wxRESIZE_BORDER )
 {
   wxBoxSizer *mainSizer = new wxBoxSizer( wxVERTICAL );

By the way, I restored this, and the YESNO dialog is still not resizable. I am missing something here.

esternin commented 3 years ago

Switchover to using persistent save/restore lost track of previously used scripts. Could you add back saving/restoring the state of the drop-down menu in Execute?

vadz commented 3 years ago

Switchover to using persistent save/restore lost track of previously used scripts. Could you add back saving/restoring the state of the drop-down menu in Execute?

This still works for me, I did check it. Do you have this (i.e. FILECOUNT and FILEn entries) in your ~/.extrema? Are they not getting restored or not saved or neither?

vadz commented 3 years ago

The inquire\yesno dialog is still of a weird size/shape and the frame is not resizable, but no buttons are obscured.

I didn't change the dialog to be resizeable, as I don't think it really needs it -- does it? The weird shape is just how it's laid out, I don't know why was it done like this, but it can be changed, of course.

Oh, I thought you were going to change the shape of the dialog to wrap around the widgets, whatever their size. If it's just static size, I can change it to something more reasonable.

Just to be clear: right now the windows do fit to their size as they use the minimum size required to lay out all their elements. In InquireDialog case the layout is somewhat weird: both top and bottom panels are verticlaly-expandable (proportion is set to 1) but in both the top nor bottom sizer there is nothing expandable. Also, both panels still use hard-coded space.

I can redo this dialog to use more standard button layout, of course, I just thought that the current layout was a conscious aesthetic choice.

This is just adding polish, and I can live with a need for the first-run resize. But I do think it would be better to accommodate this in the start-up: if ~/.extrema does not have settings (restore persistent fails), set these two windows to the 640x480 pixel and GetCharWidth(80)xGetCharHeight(20) minimum values, by doing a catch on wxPersistentRegisterAndRestore(this, "VisualizationWindow") in src/wxForms/VisualizationWindow.cpp and on wxPersistentRegisterAndRestore(this, "AnalysisWindow") in src/wxForms/AnalysisWindow.cpp.

I think for the analysis window all we have to do is to make the "Messages" box (much) higher.

As for the Visualization one, it resizes itself to use the fixed aspect ratio and I'm not sure what should it be during the first run.

esternin commented 3 years ago

Switchover to using persistent save/restore lost track of previously used scripts. Could you add back saving/restoring the state of the drop-down menu in Execute?

This still works for me, I did check it. Do you have this (i.e. FILECOUNT and FILEn entries) in your ~/.extrema? Are they not getting restored or not saved or neither?

I deleted ~/.extrema, restarted, resized, ran a script, exited. Newly created ~/.extrema does NOT contain either FILECOUNT or FILEn.

esternin commented 3 years ago

I think for the analysis window all we have to do is to make the "Messages" box (much) higher.

Sure, that would do.

As for the Visualization one, it resizes itself to use the fixed aspect ratio and I'm not sure what should it be during the first run.

Aspect RATIO is not the same as size, it defaults to 1:1.29412 (which is =11/8.5), but setting the size to 1x1.29412 (pixels?) is crazy. To match this ratio, 612:792 would work (the traditional 72dpi equivalent).

Or, can just set the min height or width, and let the aspect ratio control the rest.

vadz commented 3 years ago

I think for the analysis window all we have to do is to make the "Messages" box (much) higher.

Sure, that would do.

OK, will push soon.

As for the Visualization one, it resizes itself to use the fixed aspect ratio and I'm not sure what should it be during the first run.

Aspect RATIO is not the same as size,

Yes, of course, but it's used to set the size, see the code.

esternin commented 3 years ago

I can redo this dialog to use more standard button layout, of course, I just thought that the current layout was a conscious aesthetic choice.

Yes, please. It was not.

Also, I was not sure if the strings of the yes/no choices could be controlled. I saw "OK" being set, but it seemed like "No" was hardwired? It would be cool to add them as optional parameters. Maybe, later.

esternin commented 3 years ago

As for the Visualization one, it resizes itself to use the fixed aspect ratio and I'm not sure what should it be during the first run.

Aspect RATIO is not the same as size,

Yes, of course, but it's used to set the size, see the code.

I still think the best course of action, which will ONLY apply if there are no settings to restore from persistent storage, is to catch a fail on wxPersistentRegisterAndRestore(this, "AnalysisWindow") in src/wxForms/AnalysisWindow.cpp, and set the height to the min value of 480, and let the default (or set in persistent) aspect ratio to determine the width.

vadz commented 3 years ago

OK, I've done it like this, but using char size rather than pixels -- this should be easy to tweak if you so desire (i.e. make it bigger or smaller or different for 2 windows).

I think this one can be closed now and a separate issue be opened for redoing the layout of the inquiry dialog, as I'll do it later, according to the "plan" email, but I'm leaving closing this to you, after testing.

esternin commented 3 years ago

All good.