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

Add focus shortcuts #209

Merged
merged 5 commits into from
Mar 13, 2021
Merged

Conversation

vedgy
Copy link
Contributor

@vedgy vedgy commented Feb 11, 2021

See the commit messages for details.

@luisangelsm
Copy link
Member

Instead of disabling this in macos, we need to add a method to YACReaderMacOSXSearchLineEdit to set the focus, YACReaderMacOSXSearchLineEdit uses a NSTextField internally and we can call [field becomeFirstResponder].

@vedgy
Copy link
Contributor Author

vedgy commented Mar 13, 2021

I could try to read the docs and implement the Mac part, but I won't be able to test it as I don't have access to Mac.

@luisangelsm
Copy link
Member

It will be enough if you remove those #ifndef and add an empty method/slot to YACReaderMacOSXSearchLineEdit to gain focus, I'll commit the tested macos part after that.

vedgy added 4 commits March 13, 2021 14:59
The Ctrl+F shortcut gives focus to a search bar in many applications.
In this case it allows to search the library without touching a mouse.

YACReaderMacOSXSearchLineEdit::setFocus() will have to be implemented to
make the shortcut work on macOS.
Focusing the current comics view allows to use keyboard arrow keys to
choose among the visible comics.

The shortcut for this new action should not be a single character
without modifiers because it won't work when the search line has focus.

The Qt::FocusReason parameter in ComicsView::focusComicsNavigation()
allows to reuse this function for other keyboard navigation features.
For instance the search line can transfer focus to comics navigation
when the user presses Return or Enter key. In this case
Qt::OtherFocusReason can be used (an application-specific reason).
QWidget::sizeHint() is const-qualified, so Clang warns that non-const
sizeHint() member functions merely hide the virtual function of the base
class.

664dac3 and
9f53ae6 introduced these member
functions in 2014 without const qualifiers. QWidget::sizeHint() was
const-qualified even in Qt 3. Since these member functions have never
had any effect, they should be removed rather than const-qualified to
preserve the long-standing behaviors of the two classes.

Add a TODO for a similar but less straightforward issue with
PropertiesDialog::sizeHint().
@vedgy vedgy force-pushed the add-focus-shortcuts branch 2 times, most recently from 66c5f24 to 288b9cb Compare March 13, 2021 13:36
@vedgy
Copy link
Contributor Author

vedgy commented Mar 13, 2021

The first force-push is a rebase on the current develop. The second force-push contains the actual changes to the previous version, so reviewers can use the second force-pushed link to view the interesting diff. In addition to the requested macOS change, I have removed unused ComicsViewTransition::sizeHint() and added a TODO comment (see the third commit message).

EDIT: GitHub collapsed the two force-pushes into one and left only one force-pushed link - the latest one with the interesting diffs.

@luisangelsm
Copy link
Member

Tested on macOS, working as expected.

@luisangelsm luisangelsm merged commit 84c43e4 into YACReader:develop Mar 13, 2021
@vedgy vedgy deleted the add-focus-shortcuts branch March 13, 2021 16:10
@vedgy
Copy link
Contributor Author

vedgy commented Mar 13, 2021

By the way, I have a WIP branch where pressing Return/Enter transfers focus from the search edit to the comics view. Plus when a folder or a reading list is activated, comics view gets the focus too. In the process of implementing these features I've found numerous Library bugs, most of which are fixed in #219 and the branch mentioned in a comment to #219.

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.

2 participants