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

Ctrl+F search filter. Closes #5797. #6185

Merged
merged 2 commits into from
Jan 21, 2017

Conversation

magao
Copy link
Contributor

@magao magao commented Jan 1, 2017

This pull request addresses issue #5797.

Changes Ctrl+F to focus the torrent filter text field and select all contents, as per the comments in #5797.

@magao
Copy link
Contributor Author

magao commented Jan 8, 2017

Rebased on top of #6212 and uncrustified.

@zeule
Copy link
Contributor

zeule commented Jan 10, 2017

Do we really need the second commit ("Follow project coding style...") int this PR?

@magao
Copy link
Contributor Author

magao commented Jan 11, 2017

mainwindow.h/cpp are only modified by this PR so I could extract that out from the other one.

Alternatively, I could just make sure the new code follows the coding style, and leave the rest of mainwindow.h/cpp as-is.

@zeule
Copy link
Contributor

zeule commented Jan 13, 2017

I like this little improvement, but it would be event better if you add also a shortcut to focus files filter in the content tab, i.e. when a torrent selected, the command switches to the "Content" tab and gives focus to the filter field there.

As a Plasma user, I would suggest Ctrl + I as the default shortcut.

@zeule
Copy link
Contributor

zeule commented Jan 13, 2017

Alternatively, I could just make sure the new code follows the coding style, and leave the rest of mainwindow.h/cpp as-is.

I would prefer it this way or reformat only actually touched files (mainwindow .h and .cpp) and rearrange commits to make the reformatting first.

@magao
Copy link
Contributor Author

magao commented Jan 13, 2017

I like this little improvement, but it would be event better if you add also a shortcut to focus files filter in the content tab, i.e. when a torrent selected, the command switches to the "Content" tab and gives focus to the filter field there.

I think that should be a separate feature and pull request.

@magao
Copy link
Contributor Author

magao commented Jan 13, 2017

I would prefer it this way or reformat only actually touched files (mainwindow .h and .cpp) and rearrange commits to make the reformatting first.

Since only this commit touches these files I'll pull uncrustifying mainwindow.h/cpp to its own commit. I had felt a single uncrustify commit was the best best approach, but if it holds up merging then it's obviously not.

@zeule
Copy link
Contributor

zeule commented Jan 13, 2017

What I'm objecting to is that the uncrustifying commit is positioned last and as such is kind of useless.

@magao
Copy link
Contributor Author

magao commented Jan 13, 2017

Ah - that appears to be an artifact of how Github is presenting things. It's actually the first commit - all the other commits (and pull requests) have been rebased on top of 8501d44 (which is its own pull request #6212 so can be merged separately before all the others).

Might also partially be because I use Mercurial (via the hg-git extension) which preserves commit timestamps when rebasing (can't remember if Git does the same). So the other commits appear to be older than 8501d44.

So I won't do any changes to this PR for now.

@sledgehammer999
Copy link
Member

sledgehammer999 commented Jan 18, 2017

Can you uncrustify only the files you touch in this PR. Commit it to this PR the new uncrustification.
I want to take in the uncrustification piece-by-piece.

@ngosang ngosang self-requested a review January 21, 2017 16:43
Copy link
Member

@ngosang ngosang left a comment

Choose a reason for hiding this comment

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

Tested in Linux and it works as expected.

@sledgehammer999
Copy link
Member

Yeah, I am just waiting for @magao to renew his 2nd commit.

@magao
Copy link
Contributor Author

magao commented Jan 21, 2017

@sledgehammer999 Rebased this PR on top of master. So it's got an uncrustify commit for mainwindow.cpp/h then a commit for the Ctrl+F changes.

@sledgehammer999
Copy link
Member

Thank you.

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.

4 participants