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

Enable up/down & enter key in Welcome window (issue #3669) #3739

Merged
merged 7 commits into from
May 27, 2022

Conversation

svobs
Copy link
Contributor

@svobs svobs commented May 12, 2022


Description:
[second attempt - prev PR got deleted when I renamed the branch]
The Welcome window actually is displaying a button with the most recently played file, followed by a table of Recent File 2, Recent File 3, etc. The reason the up/down arrow keys caused a file to play was that the "selectionChanged" callback was being used as a proxy for detecting a user event.

I changed this so that:

  • When window opens, we "pretend" the button with the most recent entry is selected, which makes sense because the table has no items selected. Thus, ENTER or RETURN at this point will open the most recent file, or if for some reason that is no longer available, tries to open the next most recent one.
  • User can use UP and DOWN arrow keys to select recent files, which will highlight in the table, and then press ENTER or RETURN to open that file.
  • Mouse clicks behave the same as before

One snag: once the user scrolls down to Recent File 2 or below, they can't scroll back up to Most Recent File. That's due to the fact that that entry is actually outside the table, and would have required a bunch of custom code to make it look good. Which I was prepared to do, but I also ran into a problem where once I give the table control of the arrow keys, it doesn't seem to want to give them back, and I'd need to intercept them to make that work. So maybe that's something for the future, but this should be much improved over the previous behavior.

@low-batt
Copy link
Contributor

On this:

One snag: once the user scrolls down to Recent File 2 or below, they can't scroll back up to Most Recent File.

That sounds like a problem that needs to be fixed. However, I pulled the PR locally and tested and did not encounter that issue? I was able to scroll to the end and back up to the beginning without any problems. All seemed to be working well. What did I miss?

@svobs
Copy link
Contributor Author

svobs commented May 15, 2022

One snag: once the user scrolls down to Recent File 2 or below, they can't scroll back up to Most Recent File.

That sounds like a problem that needs to be fixed. However, I pulled the PR locally and tested and did not encounter that issue? I was able to scroll to the end and back up to the beginning without any problems. All seemed to be working well. What did I miss?

Welcome-Screen_Recently-Played-List

Here's a screenshot of the recently used items, where "1" = last recently used, "2" = 2nd to last recently used, etc. As you can see, 1 is displayed differently than the rest - it's actually a button made to look like a table entry. Only 2 and below are actually in a table.

It actually wouldn't be too hard for me to put everything in a table, and add icons and/or time listings for the rest same as the "first", or simply make it look similar to how it looks now (maybe change the table highlight color to gray, to match LRU item 1). I spent a great deal of time on another personal project recently which does that. But that seemed out of scope for this bug request where the requestor just wanted keyboard navigation for scripting. Changing everything into one table would probably actually be less work than trying to make the button behave like the first entry in the table. But I'm willing to do whatever will get approved 😉

@low-batt
Copy link
Contributor

My confusion continues as this is what I see when the window first comes up:
issue-3669
No highlighting, no button?

@low-batt
Copy link
Contributor

That is not with your current changes. I'll update.

@svobs
Copy link
Contributor Author

svobs commented May 15, 2022

No highlighting, no button?

Interesting! Does that list match the list in File > Open Recent... ? Are you using Apple Silicon?
I made some style changes early this morning so that the highlight color of the list matches that first value. But if you weren't seeing that button already, then I guess getting that straightened out is the most important issue.

@low-batt
Copy link
Contributor

Does that list match the list in File > Open Recent... ?

Yes.

Are you using Apple Silicon?

Yes and No.

I recently upgraded my development Mac. I have a model MacBookPro18,2, one of the new 16" Macs. I'm running macOS 12.3.1. I'm building with Xcode 13.3. So usually building and testing a native Apple Silicon version of the app.

I ran some tests...

Running IINA 1.1.2 on my development machine (under Rosetta) I do not see the button. It looks just like I posted above.

Under my Apple Silicon Mac I configured Xcode to build for Intel and built the current develop branch. I took the resulting app over to my older Intel MacBook Air running macOS 10.15.7. The welcome screen showed the button.

Running that same app under Rosetta on my development machine does not show the button.

IINA preferences could differ between those two machines. I took a very quick look at the code and didn't see it keying off a preference?

What version of macOS are you testing with?

@low-batt
Copy link
Contributor

Maybe this code is the issue:

    if Preference.bool(for: .recordRecentFiles),
      Preference.bool(for: .resumeLastPosition),

I will check my preference settings...

@low-batt
Copy link
Contributor

Ok. That explains the behavior. I did not have resume last position set on my development machine. Now I can reproduce the problem with not being able to get back to the first entry using up and down keys.

@low-batt
Copy link
Contributor

Not being able to up arrow back to the first entry when it is a button definitely feels wrong. Would be good to address that if possible.

@svobs
Copy link
Contributor Author

svobs commented May 15, 2022

Ok. That explains the behavior. I did not have resume last position set on my development machine. Now I can reproduce the problem with not being able to get back to the first entry using up and down keys.

Ahh good to know. I wasn't aware of that preference! I'm developing on the last Intel Macbook Pro and my knee-jerk reaction is to blame architecture differences, but surprisingly it seems there are hardly any issues with that.

I wasn't aware of that preference! But I was able to make the button disappear now. I did some testing and fortunately, none of the stuff I've added in this PR seems to misbehave when the button is gone, or if both button and table are gone.

I did push another update just an hour ago which adds hover highlight to match the other elements in the window. It was quite a bit of code but it looks a lot better. I suppose now I just need to get the UP arrow to highlight the button.

…in recentFilesTable if lastFile button not present. Prevent wraparound navigation for recent files and add system beep when already at top or bottom.
@svobs
Copy link
Contributor Author

svobs commented May 16, 2022

@low-batt I think I got it. Take a look at my latest commit and let me know what you think

Copy link
Contributor

@low-batt low-batt left a comment

Choose a reason for hiding this comment

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

I tested these changes both with resume enabled and without. All working well now.

@lhc70000
Copy link
Member

Great job! I like it very much and it works amazing.

@lhc70000 lhc70000 merged commit f111212 into iina:develop May 27, 2022
@lhc70000
Copy link
Member

I added a small commit 2dc760d to use the key names in KeyCodeHelper.keyMap when handling the key code.

svobs added a commit to svobs/iina-advance that referenced this pull request May 28, 2022
…3739)

* Enable up/down & enter key in Welcome window (issue iina#3669)

* Remove logging statements

* Improve comments

* Change highlight of table to match that of "last file" button

* Fix hover highlight

* Add hover highlight to recent files table rows

* Fix UP arrow not navigating up to "lastFile" button. Select 1st item in recentFilesTable if lastFile button not present. Prevent wraparound navigation for recent files and add system beep when already at top or bottom.
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