bugGNU Octave - Bugs: bug #53537, main_window::focus_changed()...

 
 

bug #53537: main_window::focus_changed() routine should be improved

Submitter:  Dan Sebald <sebald>
Submitted:  Sat 31 Mar 2018 09:18:30 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Other
Status:  None Assigned to:  None
Originator Name:  Open/Closed:  * Open
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 03 Apr 2018 09:16:35 PM UTC, comment #9: 

The new window doesn't get focus because, for example, the Variable Editor is empty of things that can be focused.  There is a focus issue with Documentation that needs resolving.  I found a different way of approaching Bug #53276 that doesn't use focus_changed(), so this is of less importance.  I think if the Documentation highlight is fixed, and Bug #53276 is fixed, then come back to this and I think the issue I'm describing might be more clear because the transition to toplevel widget will work as desired.

Dan Sebald <sebald>
Tue 03 Apr 2018 06:51:31 PM UTC, comment #8: 

Looks reasonable, but is it guaranteed that new_widget is the null pointer if the menu bar gets active?

Isn't it merely the question why the dragged widget does not get gocus?

Torsten Lilge <ttl>
Group Member
Tue 03 Apr 2018 06:18:14 AM UTC, comment #7: 

@Torsten:  What are your thoughts on the following test in focus_changed()?


  // catch focus changes and determine the active dock widget
  void main_window::focus_changed (QWidget *, QWidget *new_widget)
  {
    // If there is no new widget (e.g., when pressing <alt> and the global
    // menu gets active, we can return immediately
    if (! new_widget)
      return;


I'm looking at the drag-and-drop outside the main window and why that isn't always creating a top-level window.  (Recall, the drag and drop is where the newly-floated window can't immediately be turned to a top-level window while dragging.  Otherwise there is a seg fault.)  This is the one thing in Bug #53276 patch that doesn't seem to work consistently now.

I've found that when I let go of the mouse, focus_changed() does in fact get called, but sometimes the new_widget argument is nullptr because there is nothing on the dropped window to be focused.  Stated differently, the currently focused widget loses focus, but there is nothing to acquire the new focus.  This explains the rare and seemingly random behavior of sometimes a dragged, floated window doesn't become toplevel.

Does it seem that restriction should be dropped and the routine always continue on so that the currently in-focus octave_dock_widget can be un-highlighted because it lost focus even if there is no new focused widget?

There is another approach that I can take in Bug #53276 which is to temporarily install an event filter that will recognize a mouse-release event and then convert the window to a toplevel.  Then once it is toplevel, change the event filter back to the previous filter.  That way we wouldn't be relying on focus change to initiate that toplevel conversion.  Instead, we know there has to be a button release if the window is in drag state.

Dan Sebald <sebald>
Mon 02 Apr 2018 03:02:14 AM UTC, comment #6: 

The first patch left some cruft in the compilation.  New version attached, same mods as the previous.

(file #43766)

Dan Sebald <sebald>
Mon 02 Apr 2018 02:55:21 AM UTC, comment #5: 

OK, the bug.  Either before or after the previously attached changeset, float the editor window and close all other docked features.  The main window will be a gray slate.  Mouse click back and forth between the main window and the Editor window and notice that the Alt-<shortcut> isn't modified (i.e., the little underlines of _F_ile, etc.  That is, this code doesn't get executed:


        if (edit_dock_widget == dock)
          emit editor_focus_changed (true);
        else if (edit_dock_widget == m_active_dock)
          emit editor_focus_changed (false);


because when all is closed, clicking on the main window means dock=0 in this routine.

The logic shouldn't be too difficult to address (maybe check "this" against w_new or something).  Depends if one feels this is a bug or one of much importantance.

Dan Sebald <sebald>
Mon 02 Apr 2018 02:44:07 AM UTC, comment #4: 

I'm attaching a changeset that simplifies the routine by leaving out code and no real change in logic.  There's still a bug, which I will describe in a follow-up post.

1) I find the outer loop that looks at next widget in focus change to be unnecessary.

2) The m_editor_window is a pointer to a file_editor_interface, and


namespace octave
{
  class file_editor_interface : public octave_dock_widget


so there is no need to cast m_editor_window.

3) I don't see what the second expression in (dock || m_active_dock != edit_dock_widget) adds, so removed it.  I can't think of a corner case, but maybe there is one.

(file #43765)

Dan Sebald <sebald>
Mon 02 Apr 2018 12:32:33 AM UTC, comment #3: 

Will the following crash if dock=0?


        QList<QDockWidget *> tabbed = tabifiedDockWidgets (dock);


or will it return a nullptr? ... If the tabifiedDockWidgets (NULL) doesn't fail, the certainly


        if (tabbed.contains (m_active_dock))
          dock->set_predecessor_widget (m_active_dock);


will crash when tabbed = nullptr.

Dan Sebald <sebald>
Mon 02 Apr 2018 12:02:35 AM UTC, comment #2: 

After having tested a few things, I actually think the whole count-and-next-in-chain mechanism can be dropped all together.  I've printed out count--it never goes above 0 no matter where I click or tab to.  For example, the tables and lists QObjects have individual fields that can tabbed to/from but those are not individual QWidgets that exist inside, say, the bigger list.  Even if we imagine they were, I don't see the point because even a small table of 21 x 21 would already surpass 100 by the next-in-chain-try-again approach.

Plus, this is more of an organizational thing, i.e., the fact that we really should give proper ancestry for every object rather than be setting parent=0 for anything but the intended top-level windows.  I guess what I'm saying is that any scenario that the next-in-chain-try-again mechanism is meant to catch really should be avoided in the first place with good organization.  If we were to take that mechanism out and just leave the core


    // Go through all dock widgets and check whether the current widget
    // widget with focus is a child of one of it
    foreach (octave_dock_widget *w, w_list)
      {
        if (w->isAncestorOf (w_new))
          dock = w;
      }


things would be fine, and if a problem does arise just go and fix the ancestry in a logical way.  The title bar highlight for any particular case is not a severe bug if something is missed in the short term...and like I said, I've yet to see a case where count goes above 0.

I would say there are a few things to consider here, though.  The main thing is that the tab-chain doesn't seem real good as far as moving around, but I think this sort of thing is too much work and low severity for 4.4.  For example (and this is true of other windows that have lists or tables), the variable editor will highlight/focus a variable_dock_widget, i.e., subpanel, but when I press tab the chain goes inside the table.  From there, one is stuck inside the table tabbing from one cell to the next.  I'd actually prefer the outer level tab chain not go inside the table but instead jump to the next variable_dock_widget, e.g., "var1" tabs to "var2" tabs to "var3", etc.  Mouse clicking into the table would then change the tab action to tab around within the table.  This should be achievable because there can be more than one tab chain (see http://doc.qt.io/qt-5/qgraphicswidget.html#setTabOrder ).  It would nice if various levels had their own tab chain.  Say

1st level tab chain: octave_dock_widget

2nd level tab chain: within the octave_dock_widget, doesn't go from one octave_dock_widget to the next

3rd level tab chain: within items on octave_dock_widget, primarily defined by the type of object, e.g., tab, list, qscintilla-editor, etc.

and then have shift-tab go a level deeper and cntrl-tab go a level higher.  Or something like that.

The other item is I'd like you to take a look at this code:


    // if new dock has focus, emit signal and store active focus
    // except editor changes to a dialog (dock=0)
    if ((dock || m_active_dock != edit_dock_widget) && (dock != m_active_dock))
      {
        // signal to all dock widgets for updating the style
        emit active_dock_changed (m_active_dock, dock);

        QList<QDockWidget *> tabbed = tabifiedDockWidgets (dock);
        if (tabbed.contains (m_active_dock))
          dock->set_predecessor_widget (m_active_dock);

        if (edit_dock_widget == dock)
          emit editor_focus_changed (true);
        else if (edit_dock_widget == m_active_dock)
          emit editor_focus_changed (false);

        m_active_dock = dock;
      }


Perhaps the following scenario does not arise, but what if dock=0 but m_active_dock != edit_dock_widget?  That is what if

(dock || m_active_dock != edit_dock_widget)

passes, but dock=0?  Will the following crash if dock=0?


        QList<QDockWidget *> tabbed = tabifiedDockWidgets (dock);


or will it return a nullptr?

Dan Sebald <sebald>
Sun 01 Apr 2018 09:22:05 PM UTC, comment #1: 

Dan, thanks for having a look into the routine.

The max. count should be a last measure for preventing an endless loop while searching for the main dock widget that contains the element currently having the focus. In most of the cases, the first condition is already met.

If there are possibilities to make the routine more robust, I would be happy to push a related changeset.

Torsten Lilge <ttl>
Group Member
Sat 31 Mar 2018 09:18:30 PM UTC, original submission:  

I'm adding this bug report because of a bug I'm observing in the patch/changeset here

https://savannah.gnu.org/bugs/?53276#comment62

in which a new drag-and-drop feature doesn't work quite right 100% of the time.  The logic for the new feature is based upon focus, in particular when the signal


        // signal to all dock widgets for updating the style
        emit active_dock_changed (m_active_dock, dock);


should be emitted.

This has made me look into the main_window::focus_changed() routine.  Parts of this look like they could be done better.  This loop comes to mind:


    octave_dock_widget *dock = nullptr;
    QWidget *w_new = new_widget;  // get a copy of new focus widget
    QWidget *start = w_new;       // Save it as start of our search
    int count = 0;                // fallback to prevent endless loop

    QList<octave_dock_widget *> w_list = dock_widget_list ();

    while (w_new && w_new != m_main_tool_bar && count < 100)
      {
        // Go through all dock widgets and check whether the current widget
        // widget with focus is a child of one of it
        foreach (octave_dock_widget *w, w_list)
          {
            if (w->isAncestorOf (w_new))
              dock = w;
          }

        if (dock)
          break;

        // If not yet found (in case w_new is not a childs of its dock widget),
        // test next widget in the focus chain
        w_new = qobject_cast<QWidget *> (w_new->previousInFocusChain ());

        // Measures preventing an endless loop
        if (w_new == start)
          break;  // We have arrived where we began ==> exit loop
        count++;  // Limited number of trials
      }


There are a few things:

1) This count < 100 construct is strange.  Am I following correctly that this loop goes around and around the focus chain until it reaches 100?  Certainly that can be done better.  Could there possibly be more than 100 objects if the GUI gets really busy?

2) The previousInFocusChain may apply to tab order.  I think it is possible for some objects to be focusable but not tab-focusable, i.e., focusPolicy() setting.  So, generally speaking, this isn't a good approach.

3) The choice to exit the loop when w_new != m_main_tool_bar or w_new == start.  This implies a very specific ordering of where m_main_tool_bar and w_new exist in the chain of focus, i.e., the start (or end, depending on how one views it).  I wonder if it is possible that this loop could exit prematurely.

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 ttl (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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-04-02 sebald Attached File- Added octave-main_window_focus_change_cleanup-djs2018apr02.patch, #43766
    2018-04-02 sebald Attached File- Added octave-main_window_focus_change_cleanup-djs2018apr01.patch, #43765

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code