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

Add an API to fetch file icon #7851

Merged
merged 32 commits into from Feb 7, 2017

Conversation

Projects
None yet
7 participants
@YurySolovyov
Contributor

YurySolovyov commented Nov 2, 2016

Fixes #6397
Huge thanks to @deepak1556 and @MarshallOfSound who did most of the work and helped a lot.

TODO:

  • Add some tests on JS side
  • Make callback node-like with error-first signature
  • Make size argument an options object and use normal size by default.
  • Write docs
  • Try building it on macOS and fix if needed (need help, no Apple devices available for me)
  • Make it compile under/for Windows
@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 3, 2016

Contributor

Note to self: optional args handling

Contributor

YurySolovyov commented Nov 3, 2016

Note to self: optional args handling

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 3, 2016

Contributor

@deepak1556 I'm having troubles trying to understand how to detect when icon is empty for implementing node-style callbacks.
Can you please take a look at image-from-icon-nodeback branch?

It seems on linux/kde5 it always returns this icon for non-existent files:
image

Contributor

YurySolovyov commented Nov 3, 2016

@deepak1556 I'm having troubles trying to understand how to detect when icon is empty for implementing node-style callbacks.
Can you please take a look at image-from-icon-nodeback branch?

It seems on linux/kde5 it always returns this icon for non-existent files:
image

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 5, 2016

Contributor

Faced the problem with extraction of icons from executable files.
On windows:
API call:
image
Chrome Downloads page:
image

It seems like API call only uses mime type to get the "generic" icon

/cc @deepak1556

Contributor

YurySolovyov commented Nov 5, 2016

Faced the problem with extraction of icons from executable files.
On windows:
API call:
image
Chrome Downloads page:
image

It seems like API call only uses mime type to get the "generic" icon

/cc @deepak1556

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 5, 2016

Contributor

Found the problem: path should be normalized:
.getFileIcon('D:/DOWNLOAD/N1Setup.exe', ... gives generic one
and
.getFileIcon('D:\\DOWNLOAD\\N1Setup.exe'... gives one embedded into executable

Contributor

YurySolovyov commented Nov 5, 2016

Found the problem: path should be normalized:
.getFileIcon('D:/DOWNLOAD/N1Setup.exe', ... gives generic one
and
.getFileIcon('D:\\DOWNLOAD\\N1Setup.exe'... gives one embedded into executable

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Nov 5, 2016

Member

@YurySolovyov Pass the path through path.normalize 👍 It will do that transformation for you 😄 I'm sure there is a Chromium FilePath Util to do the same thing

Member

MarshallOfSound commented Nov 5, 2016

@YurySolovyov Pass the path through path.normalize 👍 It will do that transformation for you 😄 I'm sure there is a Chromium FilePath Util to do the same thing

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 5, 2016

Contributor

@MarshallOfSound problem with normalize is that it is in js and my code is in C++, so I have to state it in docs or expose only the binding to js and do normalization there

Contributor

YurySolovyov commented Nov 5, 2016

@MarshallOfSound problem with normalize is that it is in js and my code is in C++, so I have to state it in docs or expose only the binding to js and do normalization there

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 6, 2016

Contributor

Tests(not only the added ones) are somewhat unstable, even locally they pass and fail time-to-time

Contributor

YurySolovyov commented Nov 6, 2016

Tests(not only the added ones) are somewhat unstable, even locally they pass and fail time-to-time

Show outdated Hide outdated atom/browser/api/atom_api_app.cc
Show outdated Hide outdated docs/api/app.md
Show outdated Hide outdated docs/api/app.md
Show outdated Hide outdated spec/api-app-spec.js
@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 9, 2016

Contributor

I kind of agree that placing this API to NativeImage is more natural, especially w.r.t. running from both renderer and main processes and I'm happy to move it, but it would be nice to hear from @zcbenz.

Contributor

YurySolovyov commented Nov 9, 2016

I kind of agree that placing this API to NativeImage is more natural, especially w.r.t. running from both renderer and main processes and I'm happy to move it, but it would be nice to hear from @zcbenz.

@kevinsawicki

Left a few minor comments.

Also vendor/brigthray is showing up in the diff, it doesn't need to change as part of this pull request right? Might want to revert that change if it was unintended.

Show outdated Hide outdated atom/browser/api/atom_api_app.cc
Show outdated Hide outdated spec/api-app-spec.js
Show outdated Hide outdated spec/api-app-spec.js
Show outdated Hide outdated docs/api/app.md
Show outdated Hide outdated docs/api/app.md
@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 10, 2016

Contributor

@kevinsawicki Thanks, I'll address these as I get home. Any thoughts on where this API should belong?

Contributor

YurySolovyov commented Nov 10, 2016

@kevinsawicki Thanks, I'll address these as I get home. Any thoughts on where this API should belong?

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Nov 10, 2016

Contributor

Any thoughts on where this API should belong?

I'm not 100% sure, both nativeImage and app seem fine to me.

Contributor

kevinsawicki commented Nov 10, 2016

Any thoughts on where this API should belong?

I'm not 100% sure, both nativeImage and app seem fine to me.

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 10, 2016

Contributor

I would personally prefer having it under NativeImage. Are you ok with it @deepak1556 ?

Contributor

YurySolovyov commented Nov 10, 2016

I would personally prefer having it under NativeImage. Are you ok with it @deepak1556 ?

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Nov 11, 2016

Member

I am fine with it, but there are certain things you need to take care of. The IconManager class cannot be used as such if the api is called from render process, since it involves usage of browser process threads and the system classes created on the browser process. You would have to use IPC in that scenario.

Member

deepak1556 commented Nov 11, 2016

I am fine with it, but there are certain things you need to take care of. The IconManager class cannot be used as such if the api is called from render process, since it involves usage of browser process threads and the system classes created on the browser process. You would have to use IPC in that scenario.

Show outdated Hide outdated docs/api/app.md
assert.equal(err, null)
assert.equal(size.height, sizes.normal)
assert.equal(size.width, sizes.normal)
done()

This comment has been minimized.

@levrik

levrik Nov 11, 2016

Contributor

normal gets passed instead of large

@levrik

levrik Nov 11, 2016

Contributor

normal gets passed instead of large

This comment has been minimized.

@levrik

levrik Nov 11, 2016

Contributor

Strange... after submitting the review the position of the comment moves up.
image
image

@levrik

levrik Nov 11, 2016

Contributor

Strange... after submitting the review the position of the comment moves up.
image
image

This comment has been minimized.

@YurySolovyov

YurySolovyov Nov 11, 2016

Contributor

Fixed

@YurySolovyov

YurySolovyov Nov 11, 2016

Contributor

Fixed

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 11, 2016

Contributor

@deepak1556 any examples in existing APIs of how this IPC messaging could/should look like?

Contributor

YurySolovyov commented Nov 11, 2016

@deepak1556 any examples in existing APIs of how this IPC messaging could/should look like?

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Nov 15, 2016

Member

@YurySolovyov none of the common apis currently are dependent on ipc for implementation. You could probably define the api in lib/common/api/native-image.js and depending on the process either call app.GetFileIcon directly or send ipc, which is something users can do. I am not sure about the location of this api, will defer to @zcbenz

Member

deepak1556 commented Nov 15, 2016

@YurySolovyov none of the common apis currently are dependent on ipc for implementation. You could probably define the api in lib/common/api/native-image.js and depending on the process either call app.GetFileIcon directly or send ipc, which is something users can do. I am not sure about the location of this api, will defer to @zcbenz

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 15, 2016

Contributor

@deepak1556 maybe I can expose API as binding to App, and then call it directly or via IPC depending on the process as you suggested.

Contributor

YurySolovyov commented Nov 15, 2016

@deepak1556 maybe I can expose API as binding to App, and then call it directly or via IPC depending on the process as you suggested.

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 15, 2016

Contributor

How about adding it to shell? Seems more appropriate than app and native-image

Contributor

YurySolovyov commented Nov 15, 2016

How about adding it to shell? Seems more appropriate than app and native-image

@alexstrat

This comment has been minimized.

Show comment
Hide comment
@alexstrat

alexstrat Dec 28, 2016

Contributor

Hey, I'm interested in this API: how can I help? What is remaining to be done?

Contributor

alexstrat commented Dec 28, 2016

Hey, I'm interested in this API: how can I help? What is remaining to be done?

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Dec 28, 2016

Contributor

@alexstrat it is awaiting for some advice form @zcbenz to decide where this API belongs.

Contributor

YurySolovyov commented Dec 28, 2016

@alexstrat it is awaiting for some advice form @zcbenz to decide where this API belongs.

@vutran

This comment has been minimized.

Show comment
Hide comment
@vutran

vutran Feb 4, 2017

Any updates on which API this belongs to? Looking forward to implement this on my app.

vutran commented Feb 4, 2017

Any updates on which API this belongs to? Looking forward to implement this on my app.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 7, 2017

Contributor

Thanks for this @YurySolovyov, @deepak1556, and @MarshallOfSound 👍 🚢 🚀

Contributor

kevinsawicki commented Feb 7, 2017

Thanks for this @YurySolovyov, @deepak1556, and @MarshallOfSound 👍 🚢 🚀

@kevinsawicki kevinsawicki merged commit 4630f44 into electron:master Feb 7, 2017

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
@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Feb 7, 2017

Contributor

Huge thanks to @MarshallOfSound, @deepak1556 for help and @kevinsawicki for finishing this up.
Can't wait to see this in the release.

Contributor

YurySolovyov commented Feb 7, 2017

Huge thanks to @MarshallOfSound, @deepak1556 for help and @kevinsawicki for finishing this up.
Can't wait to see this in the release.

@YurySolovyov YurySolovyov deleted the YurySolovyov:image-from-icon branch Feb 7, 2017

@YurySolovyov YurySolovyov restored the YurySolovyov:image-from-icon branch Feb 8, 2017

@KELiON KELiON referenced this pull request Mar 20, 2017

Merged

Windows icons support #198

@YurySolovyov YurySolovyov deleted the YurySolovyov:image-from-icon branch Jan 8, 2018

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