Auto-Type: Remember previous selected global match #7129
Conversation
hifi
force-pushed
the
feature/autotype-remember-last
branch
from
b5f2d6a to
8b039e2
Compare
Codecov Report
@@ Coverage Diff @@
## develop #7129 +/- ##
===========================================
- Coverage 64.21% 64.16% -0.05%
===========================================
Files 334 334
Lines 42221 42257 +36
===========================================
+ Hits 27109 27112 +3
- Misses 15112 15145 +33
Continue to review full report at Codecov.
|
src/autotype/AutoTypeMatchView.h
Outdated
| @@ -35,7 +35,7 @@ class AutoTypeMatchView : public QTableView | |||
| explicit AutoTypeMatchView(QWidget* parent = nullptr); | |||
| AutoTypeMatch currentMatch(); | |||
| AutoTypeMatch matchFromIndex(const QModelIndex& index); | |||
| void setMatchList(const QList<AutoTypeMatch>& matches, bool selectFirst); | |||
| void setMatchList(const QList<AutoTypeMatch>& matches, int selectedIndex, bool selectFirst); | |||
There was a problem hiding this comment.
selectFirst looks redundant with selectedIndex.
There was a problem hiding this comment.
It's not actually redundant. selectedIndex is an index to the match list and not an index to the sorted view. I need to be able to distinguish from selecting the first visible item in the list and the first item in the input list. If it can be easily compressed into a single argument that makes sense I'll gladly refactor it.
There was a problem hiding this comment.
Then maybe just a rename to clarify the difference? Something like matchIndex or inputIndex or selectedMatch for selectedIndex, and selectFirstVisible for selectFirst?
(I'm still not entirely clear what selectedIndex actually does. May need to dig a little deeper in the code.)
There was a problem hiding this comment.
We discussed this on Matrix with @droidmonkey and decided to split this into three distinct functions where setMatchList just clears the selection so the call isn't too overloaded.
There was a problem hiding this comment.
I've now split it into separate functions.
|
@michaelk83 You don't need to setup anything special for Auto-Type sequences for this to work, it will just try its best to remember your last selection when you re-open the dialog with your global hotkey. A mention of the flow in user guide would be good I presume. |
src/autotype/AutoTypeMatchModel.h
Outdated
| @@ -49,6 +49,7 @@ class AutoTypeMatchModel : public QAbstractTableModel | |||
| QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override; | |||
|
|
|||
| void setMatchList(const QList<AutoTypeMatch>& matches); | |||
| QList<AutoTypeMatch>* matchList(); | |||
There was a problem hiding this comment.
I don't like this at all, we should not be returning a pointer to an internal data member. This indicates an antipattern (also Qt does a shallow copy of container objects). Instead you should create a function (or use an existing) that searches the match list for the provided match and returns the index.
There was a problem hiding this comment.
Added a new one since we are fine with a match that matches only the entry but not the sequence.
hifi
force-pushed
the
feature/autotype-remember-last
branch
from
fe3a882 to
fe3d3ad
Compare
droidmonkey
force-pushed
the
feature/autotype-remember-last
branch
from
1e6839f to
3c3fa2d
Compare
src/autotype/AutoTypeMatchModel.cpp
Outdated
| { | ||
| int row = -1; | ||
|
|
||
| for (int i = m_matches.size() - 1; i >= 0; i--) { |
| m_ui->searchCheckBox->setDisabled(m_matches.isEmpty()); | ||
|
|
||
| m_ui->view->setMatchList(m_matches, !m_matches.isEmpty() || !m_ui->search->text().isEmpty()); | ||
| m_ui->searchCheckBox->setChecked(m_matches.isEmpty()); | ||
| // changing check also performs search | ||
| if (m_ui->searchCheckBox->isChecked() != m_matches.isEmpty()) { | ||
| m_ui->searchCheckBox->setChecked(m_matches.isEmpty()); |
There was a problem hiding this comment.
We use m_matches.isEmpty() three times, lets give it a variable name for ease of reading: bool noMatches = m_matches.isEmpty()
I also have to read this if statement several times to understand what is going on. I think we should just always set the search checkbox here to the noMatches boolean and get rid of the if statement. We could suppress the checkbox signals to prevent a double search.
hifi
force-pushed
the
feature/autotype-remember-last
branch
2 times, most recently
from
aa03ca9 to
89d8455
Compare
This makes using multi-stage login forms slightly easier as you can avoid typing the search terms multiple times.
hifi
force-pushed
the
feature/autotype-remember-last
branch
from
89d8455 to
9b2b0d4
Compare
This makes using multi-stage login forms slightly easier as you can avoid typing the search terms multiple times. The last match is invalidated after 30 seconds which should be plenty if you are going through a login form.
Inspired by #5864 (comment) to avoid needing a complex sequence in the first place.
Testing strategy
Manually on Linux.
Type of change