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


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

Submitted by:  Dan Sebald <sebald>
Submitted on:  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

Add a New Comment(Rich Markup)

You are not logged in

Please log in, so followups can be emailed to you.


( 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>
Project Member
Tue 03 Apr 2018 06:18:14 AM UTC, comment #7:

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

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:

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

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?

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

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

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:

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?

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>
Project 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


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

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:

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:


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)

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project 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.4