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

Preview file with QuickLook #7592

Merged
merged 18 commits into from Oct 26, 2016

Conversation

Projects
None yet
8 participants
@lauracpierre
Contributor

lauracpierre commented Oct 13, 2016

Hi!

I've never done a PR here so apologies in advance if I'm missing things.

  • Linter is valid
  • Some tests are failing but it doesn't feel like this is due to my code.

I added some documentation too.

A few things I'm unsure about:

  • Not sure if I have to do anything for other platforms.
  • Not sure about memory.
- (void)previewFileAtPath:(NSString *)filepath  withName:(NSString *) name {
  NSURL * url = [[NSURL alloc] initFileURLWithPath:filepath];
  [self setQuickLookItem:[[AtomPreviewItem alloc] initWithURL:url title:name]];
  [[QLPreviewPanel sharedPreviewPanel] makeKeyAndOrderFront:nil];
}

Should I release the url here?

Happy to fix all the issues, and thanks for the feedbacks,

Pierre

screenshot 2016-10-12 21 24 51

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts Oct 13, 2016

Contributor

This is great, good work @lauracpierre - I'm not sure about the autorelease semantics either, I'll let @zcbenz weigh in

Contributor

paulcbetts commented Oct 13, 2016

This is great, good work @lauracpierre - I'm not sure about the autorelease semantics either, I'll let @zcbenz weigh in

@bengotow

This comment has been minimized.

Show comment
Hide comment
@bengotow

bengotow Oct 14, 2016

Contributor

This looks great - really excited to have this API in Electron.

Contributor

bengotow commented Oct 14, 2016

This looks great - really excited to have this API in Electron.

@lauracpierre

This comment has been minimized.

Show comment
Hide comment
@lauracpierre

lauracpierre Oct 18, 2016

Contributor

Hi @MarshallOfSound just curious to see what's the next step here, and how often you are carving new releases for Electron?

Contributor

lauracpierre commented Oct 18, 2016

Hi @MarshallOfSound just curious to see what's the next step here, and how often you are carving new releases for Electron?

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Oct 18, 2016

Member

Heh, I'm not the one to ask about that 👍

@zcbenz needs to look over / approve / merge.

Member

MarshallOfSound commented Oct 18, 2016

Heh, I'm not the one to ask about that 👍

@zcbenz needs to look over / approve / merge.

@kevinsawicki kevinsawicki self-assigned this Oct 24, 2016

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Oct 24, 2016

Contributor

Looks like commit db92afe bumped the vendor/brightray submodule, mind reverting that change?

Contributor

kevinsawicki commented Oct 24, 2016

Looks like commit db92afe bumped the vendor/brightray submodule, mind reverting that change?

Show outdated Hide outdated atom/browser/native_window_mac.mm
Show outdated Hide outdated atom/browser/native_window_mac.mm
Show outdated Hide outdated docs/api/browser-window.md
@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Oct 25, 2016

Contributor

Because of the submodule change in db92afe this branch no longer merges with master, please rebase the submodule change out of this commit.

Contributor

kevinsawicki commented Oct 25, 2016

Because of the submodule change in db92afe this branch no longer merges with master, please rebase the submodule change out of this commit.

@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz Oct 25, 2016

Contributor

It seems that the QLPreviewPanel is not bound to NSWindow? If so this should be an API of shell module, instead of being a method of BrowserWindow.

Contributor

zcbenz commented Oct 25, 2016

It seems that the QLPreviewPanel is not bound to NSWindow? If so this should be an API of shell module, instead of being a method of BrowserWindow.

@bengotow

This comment has been minimized.

Show comment
Hide comment
@bengotow

bengotow Oct 25, 2016

Contributor

Hey! It's a bit strange, but I think attaching the preview item to the window is correct. The QLPreviewPanel automatically updates itself by searching the focused window's responder chain / view hierarchy for a responder that implements QLPreviewPanelDataSource. It's sort of subtle because you never explicitly pass items to Quicklook, just open / close the panel and say "here's the preview item for my focused content."

That means the item in the quicklook preview is based on the focused window and what it's focused element provides, so clicking back and forth between two windows actually changes the preview. (I actually do this all the time by accident... 😕 )

oct-24-2016 23-57-47

Contributor

bengotow commented Oct 25, 2016

Hey! It's a bit strange, but I think attaching the preview item to the window is correct. The QLPreviewPanel automatically updates itself by searching the focused window's responder chain / view hierarchy for a responder that implements QLPreviewPanelDataSource. It's sort of subtle because you never explicitly pass items to Quicklook, just open / close the panel and say "here's the preview item for my focused content."

That means the item in the quicklook preview is based on the focused window and what it's focused element provides, so clicking back and forth between two windows actually changes the preview. (I actually do this all the time by accident... 😕 )

oct-24-2016 23-57-47

@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz Oct 25, 2016

Contributor

Hey! It's a bit strange, but I think attaching the preview item to the window is correct. The QLPreviewPanel automatically updates itself by searching the focused window's responder chain / view hierarchy for a responder that implements QLPreviewPanelDataSource.

That makes sense 👍 , thanks for the clarification.

Contributor

zcbenz commented Oct 25, 2016

Hey! It's a bit strange, but I think attaching the preview item to the window is correct. The QLPreviewPanel automatically updates itself by searching the focused window's responder chain / view hierarchy for a responder that implements QLPreviewPanelDataSource.

That makes sense 👍 , thanks for the clarification.

@lauracpierre

This comment has been minimized.

Show comment
Hide comment
@lauracpierre

lauracpierre Oct 25, 2016

Contributor

Yes thank you @bengotow and I agree this should stay on the Window.

Will do the rebase now

Contributor

lauracpierre commented Oct 25, 2016

Yes thank you @bengotow and I agree this should stay on the Window.

Will do the rebase now

@lauracpierre

This comment has been minimized.

Show comment
Hide comment
@lauracpierre

lauracpierre Oct 25, 2016

Contributor

OK I did the rebase in my fork with git rebase electron/master but now it shows all these other commits.
Was this what you where expecting @kevinsawicki ?

Contributor

lauracpierre commented Oct 25, 2016

OK I did the rebase in my fork with git rebase electron/master but now it shows all these other commits.
Was this what you where expecting @kevinsawicki ?

@lauracpierre

This comment has been minimized.

Show comment
Hide comment
@lauracpierre

lauracpierre Oct 25, 2016

Contributor

It doesn't seem to be compiling anymore...

In file included from ../../atom/browser/api/atom_api_cookies.cc:7:
../../atom/browser/atom_browser_context.h:46:8: error: incomplete type 'net::TransportSecurityState' named in nested name specifier
  net::TransportSecurityState::RequireCTDelegate* GetRequireCTDelegate()

At this stage I'm close to branch master from the current HEAD and apply my changes altogether again.

Contributor

lauracpierre commented Oct 25, 2016

It doesn't seem to be compiling anymore...

In file included from ../../atom/browser/api/atom_api_cookies.cc:7:
../../atom/browser/atom_browser_context.h:46:8: error: incomplete type 'net::TransportSecurityState' named in nested name specifier
  net::TransportSecurityState::RequireCTDelegate* GetRequireCTDelegate()

At this stage I'm close to branch master from the current HEAD and apply my changes altogether again.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Oct 25, 2016

Contributor

Was this what you where expecting @kevinsawicki ?

It wasn't, not sure what happened there, are you okay with me performing the rebase myself and force-pushing directly to your branch?

Contributor

kevinsawicki commented Oct 25, 2016

Was this what you where expecting @kevinsawicki ?

It wasn't, not sure what happened there, are you okay with me performing the rebase myself and force-pushing directly to your branch?

@lauracpierre

This comment has been minimized.

Show comment
Hide comment
@lauracpierre

lauracpierre Oct 25, 2016

Contributor

Sure thing @kevinsawicki

Contributor

lauracpierre commented Oct 25, 2016

Sure thing @kevinsawicki

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Oct 26, 2016

Contributor

Thanks @lauracpierre for adding this, it is really cool 👍 🚢

And thanks @bengotow and @MarshallOfSound for the review 👍 👀

Contributor

kevinsawicki commented Oct 26, 2016

Thanks @lauracpierre for adding this, it is really cool 👍 🚢

And thanks @bengotow and @MarshallOfSound for the review 👍 👀

@kevinsawicki kevinsawicki merged commit 9e26664 into electron:master Oct 26, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Nov 1, 2016

This is super cool! Thanks for this feature.

rgbkrk commented Nov 1, 2016

This is super cool! Thanks for this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment