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

Allow to remove account folder with Delete or Backspace keystroke #588

Merged

Conversation

MathiusD
Copy link
Contributor

When you have a large number of accounts/folder to monitor, it's quite useful to enable deletion with a keystroke on the keyboard instead of clicking a button, so this PR is made for that.

Copy link
Collaborator

@Abestanis Abestanis left a comment

Choose a reason for hiding this comment

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

I think this is a good idea, I just have an alternative proposal for the QTreeViewWithKeyEvents class.

Also, should we make the keyboard shortcut more discoverable? We could add a tooltip to the delete button that mentions the keyboard shortcuts.

Comment on lines +7 to +27
typedef void ( * TreeViewKeyPressedEvent)(void * handle, QKeyEvent * event);

/**
* A QTreeView that allow to define listener of keyEvent received
*/
class QTreeViewWithKeyEvents : public QTreeView {
Q_OBJECT

public:
explicit QTreeViewWithKeyEvents(QWidget *parent = NULL);

public slots:
void onKeyPressed(void * handleUsed, TreeViewKeyPressedEvent callback);

protected:
void keyPressEvent(QKeyEvent * event) override;

private:
TreeViewKeyPressedEvent mCallback;
void * mHandle;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we just use the Qt event system instead of building our own, that way we can avoid some casts and manually storing listeners.

Suggested change
typedef void ( * TreeViewKeyPressedEvent)(void * handle, QKeyEvent * event);
/**
* A QTreeView that allow to define listener of keyEvent received
*/
class QTreeViewWithKeyEvents : public QTreeView {
Q_OBJECT
public:
explicit QTreeViewWithKeyEvents(QWidget *parent = NULL);
public slots:
void onKeyPressed(void * handleUsed, TreeViewKeyPressedEvent callback);
protected:
void keyPressEvent(QKeyEvent * event) override;
private:
TreeViewKeyPressedEvent mCallback;
void * mHandle;
};
/**
* A QTreeView that allow to define listener of keyEvent received.
*/
class QTreeViewWithKeyEvents : public QTreeView {
Q_OBJECT
public:
explicit QTreeViewWithKeyEvents(QWidget* parent = nullptr);
signals:
void onKeyPressed(QKeyEvent* event);
protected:
void keyPressEvent(QKeyEvent* event) override;
};

with

void QTreeViewWithKeyEvents::keyPressEvent( QKeyEvent *event )
{
    emit onKeyPressed(event);
    QTreeView::keyPressEvent(event);
}

and

connect(treeAccounts, &QTreeViewWithKeyEvents::onKeyPressed, this, &DialogSettings::onKeyPressedOnTreeAccount);

Copy link
Owner

Choose a reason for hiding this comment

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

Looks fine with me. I mean, those are rare operations, hardly worthy optimizing, but as you wish :)

@gyunaev gyunaev merged commit 00110c5 into gyunaev:master Feb 20, 2024
7 checks passed
@MathiusD
Copy link
Contributor Author

I think this is a good idea, I just have an alternative proposal for the QTreeViewWithKeyEvents class.

Also, should we make the keyboard shortcut more discoverable? We could add a tooltip to the delete button that mentions the keyboard shortcuts.

Yes i can had a tooltip in another PR !

How about we just use the Qt event system instead of building our own, that way we can avoid some casts and manually storing listeners.

Yes, i have search if such signal exist in qt component and because i doesn't exist i have had own listener and i admit i've don't think to reuse signal system, i can follow your recommandation in another PR (Because it's already merged) !

MathiusD added a commit to MathiusD/birdtray that referenced this pull request Feb 21, 2024
MathiusD added a commit to MathiusD/birdtray that referenced this pull request Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants