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.

Sign up for GitHub

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

Jump to bottom

Playback History Window: add confirmation before delete from keypress, add DEL key trigger, remove {modifier}+BS triggers #4256

Merged
merged 4 commits into from Mar 25, 2023
Merged

Playback History Window: add confirmation before delete from keypress, add DEL key trigger, remove {modifier}+BS triggers #4256

merged 4 commits into from Mar 25, 2023

Conversation

svobs
Copy link
Contributor

@svobs svobs commented Mar 10, 2023

Description:
Uses KeyCodeHelper to translate like in the welcome window.

@svobs
Playback History Window: fix bug: any modifier key (except Command) d…
2e58183 
…eleted selected items. Change so that only DEL and BS keys (no modifiers) kick off delete. Also add confirmation before delete.
@low-batt low-batt linked an issue Mar 10, 2023 that may be closed by this pull request
Playback History window: no confirmation when Backspace key used #4255
Closed
@low-batt
Copy link
Contributor

Failed my testing.

Sometimes the history entry is deleted when I confirm the deletion and sometimes not. When the entry is not deleted the array selectedEntries is empty even though the entry is highlighted in blue. A quick look at HistoryWindowController shows that array is only updated in menuNeedsUpdate. Likely that needs to change.

Glad you noticed this problem. Definitely needs to be fixed.

@svobs
Copy link
Contributor Author

svobs commented Mar 13, 2023

Good catch. I pushed an update.

low-batt
low-batt approved these changes Mar 13, 2023
View reviewed changes
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.

Pulled latest commit, built IINA and tested under macOS 13.2.1. All seems to be working. Looks good to me.

@low-batt low-batt requested a review from lhc70000 March 15, 2023 02:36
uiryuu
uiryuu requested changes Mar 24, 2023
View reviewed changes
iina/HistoryWindowController.swift Outdated Show resolved Hide resolved
iina/HistoryWindowController.swift
break
}
} else {
let key = KeyCodeHelper.mpvKeyCode(from: event)
Copy link
Member

Choose a reason for hiding this comment

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

Using mpvKeyCode here is a little weird to me, although its not unacceptable, but don't we have other elegant way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well - I don't think so....It looks like MacOS's original design was never made more friendly in this regard, so I think we either have to use hexcodes or use our own constants. I was following the pattern set in the welcome window..

Copy link
Member

Choose a reason for hiding this comment

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

Maybe later on we can refactor this part, maybe to abstract another helper function...

@svobs svobs force-pushed the pr-history-window-delete-fixes branch from 2fdc19a to 9796116 Compare March 24, 2023 21:28
@uiryuu uiryuu merged commit d2cdd81 into iina:develop Mar 25, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uiryuu uiryuu uiryuu requested changes

@low-batt low-batt low-batt approved these changes

@lhc70000 lhc70000 Awaiting requested review from lhc70000

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Playback History window: no confirmation when Backspace key used
3 participants
@svobs @low-batt @uiryuu