-
Notifications
You must be signed in to change notification settings - Fork 11
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
Search with DuckDuckGo in PDF Context Menu #323
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Awesome fix, thanks for diving deep into this! 👍 💯 🥇 I only have super tiny comments about code organization that are absolutely not important :D
|
||
import Foundation | ||
|
||
final class PDFSearchTextMenuItemHandler: NSObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very tiny detail I've noticed. This class is not an extension, maybe View instead of Extensions folder is the better location?
final class MainView: NSView { | ||
private typealias CFWebServicesCopyProviderInfoType = @convention(c) (CFString, UnsafeRawPointer?) -> NSDictionary? | ||
|
||
// PDF Plugin context menu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super not important, but good practice into the future as well. Sometimes instead of comment, it's good to wrap actions inside method.
func willOpenMenu() {
editSearchMenuItem()
}
Advantages are:
- less comments
- you can add more actions into the willOpenMenu and readers understand there is no relation between them
The goal is basically to understand what is happening without the need of reading the whole class
* develop: Avoid using a specific browser as the default. (#341) Closing animation duration changed to 0.15 (#333) Hiding the favicon view when favicon is nil (#326) export bookmarks (#339) Ensure ITP database is cleared (#328) Execute print user script in page content world (#238) move the next/previous handlers to the main view controller (#331) Search with DuckDuckGo in PDF Context Menu (#323) Fix Back button takes to r.duckduckgo.com (#321) Track the existing print handler to avoid DoS attacks. (#330) Minor copy fixes (#325) support cmd-option-left/right for navigating tabs (#324) Version 0.17.5 Debounce privacy entry point update (#309) Change major tracker network threshold (#319) Check for click handler on printing user script (#318) Fix Homepage navigation (#322)
* develop: (23 commits) Update the credit card model + UI tweaks (#342) 0.17.6 Add Usage Pixel (#336) Bump find-in-page to latest version (#337) Forward delete collision with suffix resolved (#334) Avoid using a specific browser as the default. (#341) Closing animation duration changed to 0.15 (#333) Hiding the favicon view when favicon is nil (#326) export bookmarks (#339) Ensure ITP database is cleared (#328) Execute print user script in page content world (#238) move the next/previous handlers to the main view controller (#331) Search with DuckDuckGo in PDF Context Menu (#323) Fix Back button takes to r.duckduckgo.com (#321) Track the existing print handler to avoid DoS attacks. (#330) Minor copy fixes (#325) support cmd-option-left/right for navigating tabs (#324) Version 0.17.5 Debounce privacy entry point update (#309) Change major tracker network threshold (#319) ...
Task/Issue URL: https://app.asana.com/0/1199237043630360/1201320151390574/f
Tech Design URL:
CC: @tomasstrba @samsymons
Description:
Fixes "Search With %@" in PDF Plugin opening text search in Safari
Steps to test this PR:
Testing checklist:
Internal references:
Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM