Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically load selected docset on keyboard selection without enter or click #749

Closed
wants to merge 1 commit into from

Conversation

stecman
Copy link

@stecman stecman commented Jul 14, 2017

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:

zeal-update

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.

@stecman stecman changed the title Automatically load selected docsets without enter or click Automatically load selected docset on keyboard selection without enter or click Jul 14, 2017
@trollixx
Copy link
Member

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
Copy link
Author

stecman commented Jul 14, 2017

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
Copy link
Member

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
Copy link
Author

stecman commented Aug 24, 2017

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
Copy link
Member

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.


void TreeView::selectionChanged(const QItemSelection &selected, const QItemSelection &deselected)
{
QTreeView::selectionChanged(selected, deselected);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@stecman stecman Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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 stecman force-pushed the better-keyboard-selection branch 2 times, most recently from 3e483eb to 5cd78d1 Compare January 17, 2018 08:35
@stecman
Copy link
Author

stecman commented Jan 17, 2018

@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
Copy link
Member

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
Copy link
Author

stecman commented Jan 21, 2018

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 🔥

Copy link
Member

@trollixx trollixx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -568,7 +585,12 @@ void MainWindow::syncTreeView()
oldSelectionModel->deleteLater();
}

// Connect to new selection model
connect(ui->treeView->selectionModel(), &QItemSelectionModel::selectionChanged,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
        });
    }

Copy link
Author

@stecman stecman Feb 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/libs/ui/mainwindow.cpp Show resolved Hide resolved
src/libs/ui/mainwindow.h Show resolved Hide resolved
src/libs/ui/mainwindow.cpp Show resolved Hide resolved
src/libs/ui/mainwindow.cpp Show resolved Hide resolved
@stecman
Copy link
Author

stecman commented Jan 21, 2018

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
Copy link
Member

trollixx commented Jan 21, 2018

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
Copy link
Author

stecman commented Feb 25, 2018

Updated to inline the docsetSelectionChanged slot 👍

trollixx
trollixx previously approved these changes Mar 2, 2018
Copy link
Member

@trollixx trollixx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@trollixx trollixx added this to the 0.7.0 milestone Apr 17, 2018
@trollixx trollixx self-assigned this Oct 20, 2018
@trollixx
Copy link
Member

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

@stecman
Copy link
Author

stecman commented Oct 27, 2018

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
Copy link
Member

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.

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. Notably:

- Focus is no longer lost 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)
@stecman
Copy link
Author

stecman commented Nov 1, 2018

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
Copy link
Member

trollixx commented Nov 4, 2018

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

@trollixx trollixx closed this in 3118649 Dec 29, 2019
@trollixx
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants