Automatically load selected docset on keyboard selection without enter or click

[stecman]

This fixes one of my main peeves working between Dash and Zeal. Dash automatically loads the selected docset in the tree when changing the selection with the keyboard. Up until now, Zeal has required enter to be pressed to load a docset selected with the keyboard, which additionally moved focus away from the tree.

That is, in Dash the workflow is:

• search something
• use up/down arrows to move to relevant looking item
• [docset loads after a small delay]
• use up/down arrows to move to a different item
• ...

While in Zeal this has been:

• search something
• use up/down arrows to move to an item
• press enter to load selected item
• [focus moves to web view, keyboard can no longer change selection]
• use mouse to pick a different item or to focus tree again
• ...

This commit implements behaviour similar to Dash:

Notable changes:

• Focus is no longer lost (shifted to the web view) when a docset is loaded unless it is actively selected with enter or a click. This is mostly to maintain the existing behaviour for these actions. Personally I feel clicking an item should leave focus in the tree for further keyboard selection.

• The timeout for searching now serves as a general purpose delayed docset load timeout. This limits the typing and selection delays to the same value, but the 400ms currently used seems to work well in both cases.

• An "active" load (click/enter) overrides any delayed load (keyboard selection)

Other notes:

• Dash appears to throttle loading rather than use a fixed delay. Throttling results in a slightly nicer experience as changing the selection slowly loads the new selection without delay.

[trollixx]

Hi, thanks for contributing!

We are currently preparing the next release, so we can merge this once 0.4.0 is out.

Personally I like to move focus within the tree view without changing contents of the web view. How about making this behavior configurable?

The build errors require fixing, and I'll go over the code shortly.

[stecman]

CMake build fixed 👍 I was using qmake to build and test this under Linux - didn't realise CMake was in there too with a manually maintained list of source paths.

Would you consider having this behaviour as an option, but with the Dash-like behaviour the default? Not sure how many people have used Dash before, but coming from Dash the Zeal behaviour for this felt very clunky (particuarly the tree losing focus).

Also, out of curiosity, what's your process for selecting different items in the tree if this doesn't drive you nuts?

[trollixx]

Thanks for fixing CMake build. I am going to drop qmake support of the release.

I don't mind changing the default behavior, and most likely other users wouldn't start complaining about that.

I am not a super active Zeal user myself, and usually do several searches during my work day, so I just move focus with mouse. But I agree, focus moving to the web view after each activation can cause frustration. Considering we don't have an easy way to focus different areas. For that we need to implement behavior similar to F6 in major browsers (somewhat related to #401).

[stecman]

How about making this behavior configurable?

@trollixx I've been mulling over this for a while (and using a build with branch this daily) and having this as a preference still seems unnecessary to me. I generally avoid adding preferences though, so this might just be my own bias (and workflow).

What do you reckon about having a modifier like Alt ↑/↓ to change selection without loading instead? (On closer inspection, Dash has this behaviour too).

[trollixx]

I will try out your change myself, but at the moment it feels like a configuration switch would be convenient. Also having a way to reverse the configured behavior via some key combination seems to be not a bad idea.

[trollixx]

There's no need in this TreeView class. Selection updates can be obtained via QAbstractitemView::selectionModel.

[stecman]

See the comment below that - it's the first approach I tried, but it doesn't work reliably:

QTreeView doesn't normally expose selection changes as a public signal. QTreeView's selection model is accessible publicly, however the instance this refers to can change internally, so it's not reliable to connect these QItemSelectionModel signals externally.

The selection model gets switched out regularly in the current implementation, so you end up with a pointer to an old selection model after the first search and the connection no longer reports selection changes.

[trollixx]

I am still interested in this, it's just that I have some doubts about the implementation. Please bear with me, while I find some spare time to play with this.

[stecman]

@trollixx I've updated this to remove the TreeView subclass. Found the selection model was being replaced every time TreeView::setModel was called, so I've moved the selectionChanged connect to MainWindow::syncTreeView.

Also rebased against master 👍

[trollixx]

While reading about selection models, I realized that the ones replaced after setModel() are kept until the application exits. There's an unresolved QTBUG-49966 report about this situation.

I've fixed the problem in 1433063, so this PR needs another update, sorry :)

[stecman]

Rebased, thanks. I noticed the selection model didn't seem to be cleaned up when looking at the Qt source for this, but didn't look into it further 🔥

[trollixx]

A few suggestions how to make this change set even smaller.

[trollixx]

Connection should be done only for the new selection model. Plus there's really no need in the docsetSelectionChanged slot. A lambda will do just fine.
I suggest rewriting this the following way:

    // TODO: Remove once QTBUG-49966 is addressed.
    QItemSelectionModel *newSelectionModel = ui->treeView->selectionModel();
    if (newSelectionModel != oldSelectionModel) {
        if (oldSelectionModel) {
            oldSelectionModel->deleteLater();
        }

        connect(newSelectionModel, &QItemSelectionModel::selectionChanged,
                this, [this](const QItemSelection &selected) {
            if (selected.isEmpty()) {
                return;
            }

            m_openDocsetTimer->start();
            m_openDocsetTimer->setProperty("index", selected.indexes().first());
        });
    }

[stecman]

Indeed, then there's no escape from two slots :)

I'll still inline this one though, then this should be good to go I think.

[stecman]

Thanks, I'll sort this out.

The only change I kind of disagree with is removing openAndFocusDocset, but I'll follow your recommendation. What you're suggesting is less code, but it seems pretty ugly to rely on (implicitly) missing parameters in signals to drive a UI behaviour. Plus the meaning of boolean literal arguments isn't obvious when reading the code.

[trollixx]

What you're suggesting is less code, but it seems pretty ugly to rely on (implicitly) missing parameters in signals to drive a UI behaviour.

I agree, it is not perfect, but MainWindow is already pretty messy, so I am trying to keep new changes as localized as possible, at least until I move most of the code out of one file.

Plus the meaning of boolean literal arguments isn't obvious when reading the code.

If only we had named parameters in C++.

[stecman]

Updated to inline the docsetSelectionChanged slot 👍

[trollixx]

Thanks for bearing with me! I'll merge this for 0.7.0.

[trollixx]

It's been a while, will look over this shortly, and merge for 0.7.0.

[stecman]

Cheers. The only issue I've noticed with this over the last year is closing the last tab:

1. Have one tab open
2. Select a docset -> docset is loaded
3. Close the last tab -> New tab is spawned with default page
4. -> Docset remains selected and triggers autoload in the new tab

The docset selection probably just needs to be cleared when the last tab is closed.

[trollixx]

Sounds like a regression, since without the patch selections is cleared fine.

I still want add an option to make the new behavior opt-in. It should also nicely work with the keyboard navigation proposed in #1007.

[stecman]

I've been running a pretty old version - just rebased against master and that issue is no longer present 👍

Do you want to sort out that preference yourself, or should I add it here?

[trollixx]

I'll add the settings, no worries, just not sure where exactly to put them yet...

[trollixx]

I have (finally!) implemented this functionality on top of the current code base, appears to work pretty well. Unless somebody requests a configuration option, I don't see any harm in having this always enabled.

Thank you for contributing this in the first place, and sorry for my insanely slow processing time!