Add external preview (QuickLook) support #2082
Conversation
- Adapted from Files
- Internal and external preview can't open at the same time - Always preview option doesn't control external
This comment has been minimized.
This comment has been minimized.
|
Just got a chance to test run this. It looks very good! I have a few opinions on some of your problems listed:
I don't think this works well with quick look as both windows overlap.
I think we can resolve this by greying out this option if a QuickLook installation is not detected. This can be tricky though as QuickLook is available in many different forms including the Windows app store. Instead of tracking all this down we could send a pipe message and check its status to see if quick look is available and determine to enable the option.
This was also an issue with the original PR. |
|
QuickLook functionality should be provided fully by a plugin. To achieve this we need to use this #1013, and this pr is a working POC already. |
I don't think @VictoriousRaptor was around for that. But yes as a plugin would be ideal. Also I would use this PR in favor of my older code. This is more fleshed out. |
|
Maybe best that @VictoriousRaptor just continue work as is with this one so we can keep the momentum, once I helped onesound I will come back to finishing off that pr 1013 and migrate the finished code here into a plugin. |
Ok I'll do some more tests on this PR and try to figure out the problems. |
Greying out option is good. And what if user enables this option (with QuickLook installed and running), and then quit QuickLook and preview in Flow? Should we fallback to Flow's preview? And what kind of message to tell users we are not able to launch QuickLook? Maybe a notification toast?
Not a big deal. Just manually set it once and for all. |
| @@ -871,7 +871,7 @@ public void ResetPreview() | |||
| { | |||
| if (Settings.AlwaysPreview) | |||
| { | |||
| ShowPreview(); | |||
| ShowInternalPreview(); | |||
There was a problem hiding this comment.
When AlwaysPreview is true, specify internal preview.
There was a problem hiding this comment.
This is because QL is not suitable for using with always preview right?
There was a problem hiding this comment.
This is because QL is not suitable for using with always preview right?
Yep
There was a problem hiding this comment.
What happens when we have other preview tools that is suitable?
There was a problem hiding this comment.
What happens when we have other preview tools that is suitable?
Well maybe pass always preview to plugin and let plugin decide? If plugin returns false then use internal preview.
There was a problem hiding this comment.
Done. Let me know if good to merge.
| @@ -0,0 +1,51 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
If we merge is this plugin going to be a default one?
There was a problem hiding this comment.
No I will take it out before merge. There is already a QL plugin repo in the org.
jjw24
removed
the
review in progress
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis update introduces enhancements to the Flow Launcher application, focusing on integration with external preview tools like QuickLook. Key changes involve new methods for async handling of file previews, additional boolean properties for settings, refinements in preview functionalities, and the inclusion of the QuickLook plugin with relevant configurations and resources. Changes
Sequence DiagramsequenceDiagram
participant User
participant MainViewModel
participant PluginManager
participant QuickLookHelper
participant ExternalApp
User->>MainViewModel: Request preview for a file
MainViewModel->>PluginManager: Check if external preview is enabled
PluginManager-->>MainViewModel: Returns status
alt External Preview Enabled
MainViewModel->>QuickLookHelper: OpenQuickLookAsync(path)
QuickLookHelper->>ExternalApp: Open preview for file
ExternalApp-->>QuickLookHelper: Preview opened
QuickLookHelper-->>MainViewModel: Confirm preview opened
MainViewModel-->>User: Show external preview
else
MainViewModel->>InternalPreview: Open internal preview
InternalPreview-->>MainViewModel: Confirm preview opened
MainViewModel-->>User: Show internal preview
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai pause |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
None applicable. |
jjw24
force-pushed
the
quicklook
branch
2 times, most recently
from
a0a8ec9 to
bf15591
Compare
Refined version of #859. Too many merging conflict so I just create a new branch instead. Compatible with our preview function.
Changes
FilePathforResult.Previewto indicate what file should QuickLook preview.Behavior
Preview.FilePathtry to use QuickLook to preview.Preview.FilePathpressing hotkey only triggers QuickLook.Problems
Should "Always Preview" open QuickLook?NoWhen "Always Preview" is on (which means internal preview is on), selecting another result that can use QuickLook triggers QuickLook. Looks bad.When "Use QuickLook" is on but QuickLook is not launched or installed what kind of warning should be send to users?Send a notificationNo QuickLook API to set on top. (Open selected result in quicklook #859 (comment))Manually set it.Switching from external preview result to builtin result doesn't properly trigger builtin preview.TODO
Screenshot
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation