#663 Retrieve Qt terminal size from gnuplot_qt
Attached is a first version of retrieving terminal size information from gnuplot_qt so that Qt plot windows will retain their sizes in a logical way when working with multiple windows and switching between plots.
There are still bugs in the initial patch, but they will be addressed moving forward. However, it is rather close to correct, as I see it.
Here are some salient points about the changes:
1) Some visual touch up of enumerations (just one per line--looks better).
2) There is an added entry in the command enumeration, i.e., GEReportWidgetSize. This is issued to gnuplot_qt when it is desired that the size of the widget be reported back.
3) In qt_graphics, it is mandatory that the size for term->xmax and term->ymax be retrieved from gnuplot_qt. gnuplot_qt is the only record of where the window size is. This is the major addition in this patch and makes size retention work properly. The code to do this repeats the wait/decode process. Slightly bulky but not too bad. But I think that is better than trying to somehow catch the same info in a later or forced resize. We can maybe improve this later.
4) In qt_processTermEvent(), it doesn't make sense to have:
- qt_setSize = true; - qt_setWidth = event->mx; - qt_setHeight = event->my;
because all that is being done is adjusting the size. The use of qt_setWidth and forcing qt_setSize (variables typically used in options setting), to catch the size as it comes back to the qt_graphics function is a tricky approach that never seems to work out as desired. I replaced that with the more straightforward
+ term->xmax = qt_oversampling*event->mx; + term->ymax = qt_oversampling*event->my;
5) In QtGnuplotWidget.h/cpp m_skipResize is gone. This:
- if (!parent->isVisible())
- {
- m_skipResize = true;
- m_sizeHint = s + QSize(2*m_view->frameWidth(), 2*m_view->frameWidth());
- parent->updateGeometry();
- parent->show();
- m_skipResize = false;
- }
is in the same category as #4, i.e., set some variable, call some code, then set variable back to what it was previous. This sort of thing no longer has a place in a scheme so object oriented as Qt. All of that tricky resize if different and not the first time and not m_skip is gone, reduced to the more straightforward
+ if (s != viewport->size())
{
// qDebug() << " -> resizing";
if (parent)
+ parent->resize(s + parent->size() - viewport->size());
6) I moved the hint code to the point where the gnuplot widget handles GEDone:
emit plotDone();
m_scene->processEvent(type, in);
+ m_sizeHint = m_view->viewport()->size() + QSize(2*m_view->frameWidth(), 2*m_view->frameWidth());
+ parentWidget()->updateGeometry();
+ parentWidget()->show();
It seems logical to just wait until things are done and then show the plot. This may not be perfect just yet, but conceptually I think it is an improvement.
I would say that I'm not particularly keen on the gnuplot window showing its parent window because it seems to be going the wrong direction. This factors in when the external Qt window is handed into "set term qt". What is gnuplot widget showing in that case? It really should be the parent controling visibility. We can clean this up later.
7) I placed default window size (640x480) inside the initialization of the m_viewport. That eliminates all sorts of condition tests inside qt_graphics (e.g., if first time, if set_Size is true, so on).
8) I removed one unnecessary use of dynamic_cast<>. That syntax should be used very sparingly.
I propose an update version of this patch. The main modifications with respect to the original patch are the following
1/ the code in qt_term.cpp that waits for an event from the terminal is shared with the setFont mechanism
2/ it fixes a few bugs introduced by the original patch (correct mouse coordinates, scene rect positioning after a resize)
3/ the default size if moved to QtGnuplotWindow (we do not want to force a default size to a QtGnuplotWidget that is embedded in a custom Qt application)
4/ m_skipResize and dynamic_cast<> are restored. These are arguably bad programming practice, but they were introduced for a specific reason, and plainly removing them does not make this reason disappear:
- concerning m_skipResize: I seems to me that there is no way in Qt to differentiate events that come from user interaction from events that are programmatically triggered. It this case, there is no point to notify the gnuplot core that a resize event has been triggered when this resize event precisely came from gnuplot core itself. There was even a specific situation on OSX where this causes a bug. Hence the introduction of the m_skipResize
- concerning dynamic_cast<>: the policy of the Qt terminal is to resize the widget only if it is included in a QMainWindow, and not if it is embedded in a custom Qt application that may want to control the size of the widget the way it wants, and not the way the gnuplot process wants. Whether this policy is the correct one or not can be debated, but at least dynamic_cast<QMainWindow*> does precisely what the policy says.
OK, I've tried your alternate patch.
in QtGnuplotWidget::init().
if (type == GESetCurrentWindow) in >> i; // 1000 else if (type == GEInitWindow) ; // 1001 else if (type == GECloseWindow) in >> i; // 1002 else if (type == GEExit) ; // 1003 else if (type == GEPersist) ; // 1004 else if (type == GEStatusText) in >> string; // 1005 else if (type == GETitle) in >> string; // 1006 else if (type == GESetCtrl) in >> b; // 1007 else if (type == GECursor) in >> i; // 1009 else if (type == GEZoomStart) in >> string; // 1022 else if (type == GEZoomStop) in >> string; // 1023 else if (type == GERaise) ; // 1034 else if (type == GEDesactivate) ; // 1038Those numbers are incorrect if someone puts a new enumeration in the list. The purpose of enumerations is to get rid of the intractable numbers.
The above was probably the same in my patch. It seems that not enough info, generally, is being retained...not only size info.
QMainWindow* parent = dynamic_cast<QMainWindow*>(parentWidget()); if (parent) parent->show();but the casting is unnecessary, isn't it? Doesn't C++ take the abstract class and call the function at its highest overridden level? "show()" is a member of the base class and will call the QMainWindow routine, if I recall correctly. This code is just calling the same function directly that C++ will eventually call with just an added level of abstraction.
Then there is a third dynamic_cast still in the code, but it isn't easy to remove without restructuring the files a bit. (Not the code, but the files.) First, casting in the way that was done means that the one object (handler) is assuming something about the parent that it really shouldn't assume. If the programmer passes in an object that isn't of that assumed class, then program execution fails. That's not super likely to happen, but from the programmer's perspective it is just easier to follow if dynamic_cast is avoided.
I wanted to simply pass in the QtGnuplotWidget, i.e.,
QtGnuplotEventHandler::QtGnuplotEventHandler(QtGnuplotWidget* parent, const QString& socket) : QObject(parent) { m_receiver = parent;and keep track of the receiver pointer in new variable QtGnuplotEventReceiver* m_receiver. QtGnuplotWidget is derived from two base classes (that's fine) so C++ should cast "parent" to QObject(parent) without problem and cast "parent" to m_receiver = parent without problem. But the problem this runs into is the fact that QtGnuplotEventHandler needs the QtGnuplotWidget declaration (which is in QtGnuplotWidget.h) and QtGnuplotWidget needs the QtGnuplotEventReceiver declaration (which is in QtGnuplotEvent). That's why there's a general rule of not having more than one class per file. In the case where objects are derivative of a class in the file, it is fine. But in this case QtGnuplotEventReceiver and QtGnuplotEventHandler are different branches of the hierarchy and interact with one another. They shouldn't be in the same file.
In summary, generally the changes look pretty good. You've organized things a little more with new functions, cleaned up the way y-size is computed, etc. It certainly doesn't have the issue my patch had with dropping data (probably because your new blocking function is more robust). So, I'd say go ahead and apply your patch (if you are allowed) with the possible change of getting rid of the dynamic casts. I'm still not a fan of the "m_skipResize" construct and think there should be a better way of avoiding calling the core unnecessarily, but maybe things will fall more into place down the road.
It came to mind to check what the documentation says:
So, perhaps it is the initial plot not maintaining aspect ratio which is violating the documentation.
The above is different than the x11 behavior. Should we raise this issue on the discussion list? It might not be a bad option to have "maintain aspect ratio" in one of the drop down menus of the Qt terminal. Of course, we always like to have command line access to such things too.
OK, well, it's more than that. There is this whole conflicting gnuplot aspect ratio control that makes the "active" plot window behave differently from the "inactive" plot windows. The active window behaves according to the gnuplot core aspect ration control. (See "help aspect".) The plot fills the window, the lines/fonts do not scale.
The inactive windows behave according to what the "help set term qt" documentation says--lines/fonts scale in size and aspect ratio is frozen.
I think it might be less confusing to behave just one way. We can open it up to the discussion list. (I'm pretty sure that folks will say the former scenario--i.e., fonts and lines don't scale--is the right one, but never know.)
BTW, please leave the aspect ratio issue for a separate patch and commit your recent work.