bugGNU Octave - Bugs: bug #53443, Compile warning when casting...

 
 

bug #53443: Compile warning when casting QVariant to (QWidget *)

Submitter:  Dan Sebald <sebald>
Submitted:  Fri 23 Mar 2018 04:19:40 PM UTC
   
 
Category:  Configuration and Build System Severity:  2 - Minor
Priority:  3 - Low Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 10 Apr 2018 05:26:31 AM UTC, comment #31: 

Yes, I've tested here.  I will move a freshened version of the patch from Comment #24 to a new bug report.

Dan Sebald <sebald>
Tue 10 Apr 2018 05:05:17 AM UTC, comment #30: 

I pushed a fix for the C-style cast warning on the stable branch

https://hg.savannah.gnu.org/hgweb/octave/rev/8cc3213f788f

Can we consider this issue closed now, and work on focus issues on a separate bug?

Mike Miller <mtmiller>
Group Member
Tue 10 Apr 2018 02:55:52 AM UTC, comment #29: 

Oh, I should point out that Rik applied a changeset I created a few hours ago and it partially broke Variable Editor behavior as far as focus.  You will want to apply the changeset here:

https://savannah.gnu.org/bugs/?53410#comment12

along with what you have.  I've done this here and it appears to function correctly with the reinterpret_cast.

Dan Sebald <sebald>
Tue 10 Apr 2018 02:39:35 AM UTC, comment #28: 

Thanks. Instead of the command window, I dropped the variable editor onto the file browser so they are tabbed together, and then made the file browser be on top. I then defined a few variables and ran openvar, and the variable editor did come to the front. So this looks like it's working.

Mike Miller <mtmiller>
Group Member
Tue 10 Apr 2018 02:37:47 AM UTC, comment #27: 

x = magic(5);

Dan Sebald <sebald>
Tue 10 Apr 2018 02:37:16 AM UTC, comment #26: 

Place the Command Line window on top of a tab stack along with Variable Editor.  Then type

x = magin(5);
openvar x;

The goal is to get the Variable Editor to come to the top of the stack.  It should be working currently in the stable branch.

Dan Sebald <sebald>
Tue 10 Apr 2018 02:27:56 AM UTC, comment #25: 

And on the stable 4.4 branch to fix the compiler warning? Something like this change seems a lot clearer to me than what we have now:


--- a/libgui/src/variable-editor.cc
+++ b/libgui/src/variable-editor.cc
@@ -1197,12 +1197,13 @@ namespace octave
     if (parent () != nullptr)
       {
         QList<QTabBar *> barlist = main_win ()->findChildren<QTabBar *> ();
+        QVariant this_value (reinterpret_cast<quintptr> (this));

         foreach (QTabBar *tbar, barlist)
           {
-            for (int i=0; i < tbar->count (); i++)
+            for (int i = 0; i < tbar->count (); i++)
               {
-                  if ((QWidget *) tbar->tabData (i).toULongLong () == this)
+                if (tbar->tabData (i) == this_value)
                   {
                     tbar->setCurrentIndex (i);
                     return;


It eliminates the warning for me, but does this work? What am I looking for to test that this does the right thing?

Mike Miller <mtmiller>
Group Member
Tue 10 Apr 2018 01:52:04 AM UTC, comment #24: 

Here's a proposed patch for default that gets rid of the need for the tab_to_font() routine by using the strategy that brings "focus_command_window" to the front by utilizing the focusInEvent() virtual function for reusability.  It's probably shaking the tree a bit too much for 4.4 release, but I like the approach.  It totally eliminates the tab_to_front() code.

In the current implementation is a newly added focus() virtual function.  It seems to me that's essentially serving the same role as focusInEvent() so I moved the functionality of octave_dock_widget::focus(void) to octave_dock_widget::focusInEvent(QFocusEvent *ev).  Then from there, it was merely replacing all occurrences of focus() with Qt's setFocus() routine (the setFocus will issue an event that makes its way to focusInEvent() if appropriate.  I removed a few superfluous lines of code here an there as well.

(file #43863)

Dan Sebald <sebald>
Mon 09 Apr 2018 10:46:14 PM UTC, comment #23: 

Thanks Mike.  At the moment I'm trying to make the tab come to the front similar to the way focus_command_window() is done by trying to reuse that code inside of the already virtual function focusInEvent().  That would then avoid the tab issue.  Partial success so far, but still some issues.

Dan Sebald <sebald>
Mon 09 Apr 2018 10:39:13 PM UTC, comment #22: 

Dan is right, Octave doesn't have any access programmatically to the tab bars that the QMainWindow controls. They are created on the fly as widgets are moved around and stacked on top of each other in the docked layout that we use.

I crawled through the Qt code a bit and found that the actual call to setTabData is passing a quintptr, which is Qt's version of the POSIX type uintptr_t. And the quintptr is just a reinterpret_cast<quintptr> of the address of the widget being added to the tab layout.

Mike Miller <mtmiller>
Group Member
Mon 09 Apr 2018 05:40:27 PM UTC, comment #21: 

The tbar->tabData(i) format is set internally by Qt.  We have no control of that.  I don't think Qt developers intended for there to be a programmatic set of an index on a QMainWindow.  It would all be user-driven by clicking the mouse on a tab, etc.  And the reason is probably because these QDockWidget objects don't get deleted by the absence of a QTabWidget; the just get shuffled around in terms of tabbed in a group or not tabbed in a group.  In other words, it doesn't make sense for QMainWindow to be creating and deleting QTabWidgets, just the tab bars because the QTabBar isn't a container class.

I've dumped the QMetaObject names of everything that is in the main GUI window.  It is a long list, but I don't see any QTabWidget object that corresponds to the QMainWindow-created tabs in the various areas.  (If that were the case, then we could use a class that does have several functions for identifying the widgets in the container class and select the index.  That would be easy...but that's not the case.)  Qt must have programmed it's QMainWindow so that it is merely using QTabBars and essentially replicates what QTabWidget class does.  There are probably signals/slots connections between the QTabBars--program flow we aren't privy to.

So, it seems to me the only location the information is accessible is that tbar->tabData(i).  I thought I had a good lead on a solution, which is this routine from the documentation:

http://doc.qt.io/qt-5/qtabbar.html#accessibleTabName

"Returns the accessibleName of the tab at position index, or an empty string if index is out of range."

A widget has an accessibleName:

http://doc.qt.io/qt-5/qwidget.html#accessibleName-prop

That way, I thought we could compare the accessibleTabName list against the accessibleName of "this" and avoid all the pointer stuff.  But the compiler keeps telling me that accessibleTabName() is not a function of QTabBar...plus accessibleName() returns an empty string.

Dan Sebald <sebald>
Mon 09 Apr 2018 04:17:07 PM UTC, comment #20: 

I pushed the change from comment #12 which correctly sets the focus on the V.E. window even if it was originally in a hidden state (http://hg.savannah.gnu.org/hgweb/octave/rev/4e79e324f441).

I'm changing the summary of the bug report to the remaining issue which is a compile warning; also, lowering the severity and priority.

Rik <rik5>
Group administrator
Mon 09 Apr 2018 12:14:55 PM UTC, comment #19: 

Where is the pointer value being stored as an unsigned long long value?

You should be able to avoid the warning by using reinterpret_cast instead of static_cast, but that's not really a good solution.  We shouldn't be stuffing pointers in integers.

John W. Eaton <jwe>
Group administrator
Mon 09 Apr 2018 08:07:00 AM UTC, comment #18: 

Oh.  Yeah, that doesn't appear to work.  I've printed out


    for (int i=0; i < tbar->count (); i++)
      {
std::cerr << "void* cast: " << tbar->tabData(i).value<void*> () << "\n";
        if (tbar->tabData(i).value<void*> () == this)
          {
std::cerr << "FOUND THE ONE\n";
            tbar->setCurrentIndex (i);
            return;
          }
      }


which results in


>> x = magic(5);
>> openvar x;
>> void* cast: 0
void* cast: 0
void* cast: 0
void* cast: 0
void* cast: 0
void* cast: 0


From the documentation:

http://doc.qt.io/qt-5/qvariant.html#value

"Returns the stored value converted to the template type T. Call canConvert() to find out whether a type can be converted. If the value cannot be converted, a default-constructed value will be returned."

It's returning a value of 0 (the default) because Qt isn't allowing that type of conversion.  Generally, from the looks of it, Qt doesn't like values being converted to pointers.

With that in mind, I think what I had in the changeset of Comment #11 is probably the most sound.  Converting a pointer to a ULongLong QVariant is probably not considered bad by Qt.  Somehow Qt must be doing that kind of thing in the QTabBar code when a new widget is added, and what I've done is likely how Qt developers have done it.

Now, given what I printed out--i.e., that the value<void*> is producing 0--one would think that the webinfo::current_tab_changed() routine would crash.  However, I don't see anywhere in the code where this object is actually instantiated so don't know where to look to test it.

Dan Sebald <sebald>
Mon 09 Apr 2018 07:27:52 AM UTC, comment #17: 

I think so. But note that I haven't tested any of this, I'm only building. You should verify that the GUI is functionally doing the right thing with that change.

Mike Miller <mtmiller>
Group Member
Mon 09 Apr 2018 07:25:19 AM UTC, comment #16: 

Is it that simple?  :-)  Overlooked the obvious I guess.

Dan Sebald <sebald>
Mon 09 Apr 2018 07:18:56 AM UTC, comment #15: 


> it's merely a comparison of unique numbers


I think you just revealed the simplest solution, no casting or type fooling required, just something like


if (tbar->tabData(i).value<void*> () == this)


Compiles without any warnings for me. And that seems like the clearest possible way yet to do the comparison safely.

Mike Miller <mtmiller>
Group Member
Mon 09 Apr 2018 07:11:13 AM UTC, comment #14: 

Ah, one level directory deeper than libgui/src/.  I probably didn't grep that far down.

It's not necessarily true that the tabData() pointer usage should be the same.  In the changeset I attached in Comment #11, neither the tabData information nor the "this" pointer is used as a pointer for referencing data or a function; it's merely a comparison of unique numbers.  As I see it, this is a more legitimate use of such variables than is the static_cast<>.

The problem with static_cast used as an actual pointer is that it sort of promotes programming outside the framework's concept.  That is, if we have pointers and such, one starts programming the way they would with conventional C++.  But if one avoids the casting, then it requires some thought and redesign using Qt's features of signals/slots, virtual functions, etc.

For example in this function:


01  void webinfo::current_tab_changed (int index)
02  {
03    QVariant tab_data = _tab_bar->tabData (index);
04    _text_browser = static_cast<QTextBrowser *> (tab_data.value<void*> ());
05
06    _stacked_widget->setCurrentIndex (_stacked_widget->indexOf (_text_browser));
07
08    if (_text_browser->font () != _font_web)
09      _text_browser->setFont (_font_web);
10
11    tab_state_changed ();
12  }


there really should be a check on index because it can be -1 if there are no widgets in the QTabBar.  If that were the case, then there'd be either out-of-range seg fault or a bad pointer seg fault.  And why the step of setting the QStackedWidget index?  Doesn't that information come from the QTabBar itself?  I.e., it's telling use what became the current widget.  And the possible change of font: here the _text_browser is being used like a pointer, but I wonder why the need to do this font change rather than doing so at the point the QTextBrowser is created.

Dan Sebald <sebald>
Mon 09 Apr 2018 06:37:12 AM UTC, comment #13: 

For reference, pointers to widgets are pulled out of QVariants elsewhere in libgui, it would probably be helpful to our future selves if we did it consistently in all places.

In webinfo.cc:


QVariant tab_data = _tab_bar->tabData (index);
_text_browser = static_cast<QTextBrowser *> (tab_data.value<void*> ());

QVariant tab_data = _tab_bar->tabData (index);
QWidget *w = static_cast<QWidget *> (tab_data.value<void*> ());


Mike Miller <mtmiller>
Group Member
Mon 09 Apr 2018 05:39:14 AM UTC, comment #12: 

Attached is a fix for the focus not happening when the Variable Editor is in a hidden state. The solution is just to make sure that the Variable Editor is not hidden prior to doing all the other steps. Seems to work.

(file #43851)

Dan Sebald <sebald>
Mon 09 Apr 2018 05:32:45 AM UTC, comment #11: 

"The question to me is how the developers created a QVariant of type QMetaType::ULongLong from a QWidget *?"

Got it!  See the attached changeset for the strange construct.

(file #43849)

Dan Sebald <sebald>
Mon 09 Apr 2018 04:27:28 AM UTC, comment #10: 

I printed out the "type" of the QVariant, i.e.,


std::cerr << "type: " << tbar->tabData (i).type () << "\n";


It indicates QMetaType::ULongLong, i.e., 5.  See, Qt has all these rules that the QVariant of type X can be converted to type A, D, F, etc., otherwise the value is 0.  The question to me is how the developers created a QVariant of type QMetaType::ULongLong from a QWidget *?  If one can do that, it's done.

Dan Sebald <sebald>
Mon 09 Apr 2018 03:47:25 AM UTC, comment #9: 

Here's an example of what I mean.  This discussion

https://stackoverflow.com/questions/3887064/qvariant-to-qobject

suggests a two-stage process of first converting to QObject and then convert that QObject to QWidget *.

No compiler warning, but again the pointer value comes out zero:


>> x = magic(5);
>> openvar x;
>> pobj: 0
pobj: 0
pobj: 0
pobj: 0
pobj: 0


Here's where I use the C-style cast (i.e., the Warning scenario):


>> x = magic(5);
>> openvar x;
>> pobj: 0x2442d70
pobj: 0x1f5ff80
pobj: 0x1c87890
pobj: 0x2247c70
FOUND THE WINNER


Dan Sebald <sebald>
Mon 09 Apr 2018 03:26:47 AM UTC, comment #8: 

I think I tried most of these sorts of conversions.  Converting "this" as well, I may have tried.  The C-style cast is apparently a stronger cast than the C++ casts.


/home/sebald/octave/octave/octave/libgui/src/variable-editor.cc:1203:79: error: invalid static_cast from type ‘qulonglong {aka long long unsigned int}’ to type ‘QWidget*’
          if (static_cast<QWidget *> (tbar->tabData (i).toULongLong ()) == this)
                                                                      ^
Makefile:17165: recipe for target 'libgui/src/libgui_src_libgui_src_la-variable-editor.lo' failed
make[2]: *** [libgui/src/libgui_src_libgui_src_la-variable-editor.lo] Error 1
make[2]: *** Waiting for unfinished jobs....


Dan Sebald <sebald>
Mon 09 Apr 2018 03:01:09 AM UTC, comment #7: 

Interesting, I don't get that warning, but my version of gcc is older.  I assume the change is as simple as


if (static_cast<QWidget *> (tbar)->tabData (i).toULongLong () == this)


I won't make the change because I can't verify that it fixes the problem.

Rik <rik5>
Group administrator
Mon 09 Apr 2018 02:59:15 AM UTC, comment #6: 

As I explained, I tried.  Unless I missed some Qt function, there seems to be no Qt function for identifying the tab contents other than that tabData(i) which is a QVariant.  That surprises me that there is no such routine.  In any case, one has to use the QVariant functions that will allow translating to the desired format.  Every conversion type I tried that seemed a reasonable choice for comparison would produce a translated value of 0.  If anyone has an idea of how to convert the tabData() and QWidget pointer in either direction or to a mutually-convertable type, I'm listening.

Here's the documentation that might be helpful:

http://doc.qt.io/qt-5/qvariant.html#type

"Also note that the types void*, long, short, unsigned long, unsigned short, unsigned char, float, QObject*, and QWidget* are represented in QMetaType::Type but not in QVariant::Type, and they can be returned by this function. However, they are considered to be user defined types when tested against QVariant::Type."

Dan Sebald <sebald>
Mon 09 Apr 2018 02:14:40 AM UTC, comment #5: 

This change has introduced a new warning that should be eliminated


../libgui/src/variable-editor.cc: In member function ‘void octave::variable_editor::tab_to_front()’:
../libgui/src/variable-editor.cc:1202:66: warning: use of old-style cast [-Wold-style-cast]
                   if ((QWidget *) tbar->tabData (i).toULongLong () == this)
                                                                  ^


Mike Miller <mtmiller>
Group Member
Mon 09 Apr 2018 01:58:09 AM UTC, comment #4: 

I pushed your changeset to the stable branch here (http://hg.savannah.gnu.org/hgweb/octave/rev/8c38361718a9).

If I could make one more request, is it possible to get this to work when the Variable Editor window is not displayed?


When the V.E. window or dock widget is not displayed then 'openvar' will cause it to appear, but the focus isn't shifted.  If the window or widget is already open, even if empty, then the patch works as expected.

Rik <rik5>
Group administrator
Sun 08 Apr 2018 06:31:19 AM UTC, comment #3: 

I think you are noticing something I didn't notice, which is that the focus isn't actually changing when, say, the Variable Editor is docked in the main GUI alongside the Command Line panel.  I've addressed that in the new changeset candidate.  I think I see now that setFocus() doesn't do anything unless the window is active (and the tabbed widget is visible).  I've move all setFocus() instances after activateWindow().

The main goal of this changeset was to bring the tabbed Variable Editor to the front of the tab stack.  In terms of layout, put the Command Window on top of the Variable Editor so they share the tab bar.  In the Command Window now do "openvar x;".  After the changeset, the Variable Editor should now appear on top of the Command Window (and the "x" panel focused).

(file #43836)

Dan Sebald <sebald>
Sat 07 Apr 2018 11:47:55 PM UTC, comment #2: 

I tried the patch, but I'm not sure what behavior to look for.

In the Command Window I typed


x = magic (3)
openvar x


This still left the focus in the Command Window.  Is that right, or was it supposed to shift to the V.E. and the newly opened tab?

Rik <rik5>
Group administrator
Fri 23 Mar 2018 09:54:35 PM UTC, comment #1: 

Here's a patch, simple in concept but what a bear it is to convert a QVariant to something that can be compared to a QWidget pointer.  The doc for QVariant says that a QObject pointer is fine (i.e., QMetaType::QObjectStar), but all the conversions I've tried fail.  Also, I tried turning the "this" pointer into a QVariant, which again is supposedly valid, but that too fails.  The failure going one way or another is always that the value Qt reports is 0.  I'd guess internally there may be some kind of dynamic_cast involved that claims the conversion can't be done so sets the value to zero.  qobject_cast<> complains at compile.  qvariant_cast<> complains at compile.  As it is, the use of C-cast leaves a warning message...  I'll have to try again next week.

(file #43653)

Dan Sebald <sebald>
Fri 23 Mar 2018 04:19:40 PM UTC, original submission:  

Currently, variable editor will raise to the top of the desktop when floating and the user types "openvar x".  However, when variable editor is a docked and tabbed within the GUI, that tab is not shown.  It is preferred that the tabbed version also appear.

Dan Sebald <sebald>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by sebald (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-04-10 mtmiller StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-04-10 mtmiller StatusIn Progress Ready For Test
    2018-04-10 sebald Attached File- Added octave-eliminate_focus_and_tab_to_front_functions-djs2018apr09.patch, #43863
    2018-04-09 rik5 CategoryGUI Configuration and Build System
        Severity3 - Normal 2 - Minor
        Priority5 - Normal 3 - Low
        StatusPatch Reviewed In Progress
        SummaryVariable Editor: openvar VAR should show the tab on which the variable editor is Compile warning when casting QVariant to (QWidget *)
    2018-04-09 sebald Attached File- Added octave-ve_focus_variable_from_hidden_state-djs2018apr08.patch, #43851
    2018-04-09 sebald Attached File- Added octave-tab_to_front_compiler_warning-djs2018apr08.patch, #43849
    2018-04-08 sebald Attached File- Added octave-ve_tab_to_front-djs2018apr08.patch, #43836
    2018-04-07 rik5 StatusNone Patch Reviewed
    2018-03-23 sebald Attached File- Added octave-ve_tab_to_front-djs2018mar23.patch, #43653

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code