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.

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

feat: promisify app.getFileIcon() #15742

Merged
merged 12 commits into from Dec 5, 2018
35 changes: 14 additions & 21 deletions atom/browser/api/atom_api_app.cc
Expand Up @@ -536,17 +536,15 @@ int ImportIntoCertStore(CertificateManagerModel* model,
#endif

void OnIconDataAvailable(v8::Isolate* isolate,
const App::FileIconCallback& callback,
scoped_refptr<util::Promise> promise,
gfx::Image* icon) {
v8::Locker locker(isolate);
v8::HandleScope handle_scope(isolate);

if (icon && !icon->IsEmpty()) {
callback.Run(v8::Null(isolate), *icon);
promise->Resolve(*icon);
} else {
v8::Local<v8::String> error_message =
v8::String::NewFromUtf8(isolate, "Failed to get file icon.");
callback.Run(v8::Exception::Error(error_message), gfx::Image());
promise->RejectWithErrorMessage("Failed to get file icon.");
}
}

Expand Down Expand Up @@ -1125,16 +1123,16 @@ JumpListResult App::SetJumpList(v8::Local<v8::Value> val,
}
#endif // defined(OS_WIN)

void App::GetFileIcon(const base::FilePath& path, mate::Arguments* args) {
mate::Dictionary options;
IconLoader::IconSize icon_size;
FileIconCallback callback;

v8::Local<v8::Promise> App::GetFileIcon(const base::FilePath& path,
mate::Arguments* args) {
v8::Locker locker(isolate());
v8::HandleScope handle_scope(isolate());
v8::HandleScope scope(isolate());
codebytere marked this conversation as resolved.
Show resolved Hide resolved

scoped_refptr<util::Promise> promise = new util::Promise(isolate());
base::FilePath normalized_path = path.NormalizePathSeparators();

IconLoader::IconSize icon_size;
mate::Dictionary options;
if (!args->GetNext(&options)) {
icon_size = IconLoader::IconSize::NORMAL;
} else {
Expand All @@ -1143,22 +1141,17 @@ void App::GetFileIcon(const base::FilePath& path, mate::Arguments* args) {
icon_size = GetIconSizeByString(icon_size_string);
}

if (!args->GetNext(&callback)) {
args->ThrowError("Missing required callback function");
return;
}

auto* icon_manager = AtomBrowserMainParts::Get()->GetIconManager();
gfx::Image* icon =
icon_manager->LookupIconFromFilepath(normalized_path, icon_size);
if (icon) {
callback.Run(v8::Null(isolate()), *icon);
promise->Resolve(*icon);
} else {
icon_manager->LoadIcon(
normalized_path, icon_size,
base::Bind(&OnIconDataAvailable, isolate(), callback),
&cancelable_task_tracker_);
icon_manager->LoadIcon(normalized_path, icon_size,
base::Bind(&OnIconDataAvailable, isolate(), promise),
&cancelable_task_tracker_);
}
return promise->GetHandle();
}

std::vector<mate::Dictionary> App::GetAppMetrics(v8::Isolate* isolate) {
Expand Down
3 changes: 2 additions & 1 deletion atom/browser/api/atom_api_app.h
Expand Up @@ -198,7 +198,8 @@ class App : public AtomBrowserClient::Delegate,
void ImportCertificate(const base::DictionaryValue& options,
const net::CompletionCallback& callback);
#endif
void GetFileIcon(const base::FilePath& path, mate::Arguments* args);
v8::Local<v8::Promise> GetFileIcon(const base::FilePath& path,
mate::Arguments* args);

std::vector<mate::Dictionary> GetAppMetrics(v8::Isolate* isolate);
v8::Local<v8::Value> GetGPUFeatureStatus(v8::Isolate* isolate);
Expand Down
25 changes: 23 additions & 2 deletions docs/api/app.md
Expand Up @@ -552,8 +552,29 @@ On _Windows_, there a 2 kinds of icons:
- Icons associated with certain file extensions, like `.mp3`, `.png`, etc.
- Icons inside the file itself, like `.exe`, `.dll`, `.ico`.

On _Linux_ and _macOS_, icons depend on the application associated with file
mime type.
On _Linux_ and _macOS_, icons depend on the application associated with file mime type.

**[Deprecated Soon](promisification.md)**

### `app.getFileIcon(path[, options])`

* `path` String
* `options` Object (optional)
* `size` String
* `small` - 16x16
* `normal` - 32x32
* `large` - 48x48 on _Linux_, 32x32 on _Windows_, unsupported on _macOS_.

Returns `Promise<NativeImage>` - fulfilled with the app's icon, which is a [NativeImage](native-image.md).

Fetches a path's associated icon.

On _Windows_, there a 2 kinds of icons:

- Icons associated with certain file extensions, like `.mp3`, `.png`, etc.
- Icons inside the file itself, like `.exe`, `.dll`, `.ico`.

On _Linux_ and _macOS_, icons depend on the application associated with file mime type.

### `app.setPath(name, path)`

Expand Down
9 changes: 7 additions & 2 deletions lib/browser/api/app.js
Expand Up @@ -40,9 +40,14 @@ Object.assign(app, {
}
})

const nativeFn = app.getAppMetrics
const nativeGetFileIcon = app.getFileIcon
app.getFileIcon = (path, options = {}, cb) => {
return deprecate.promisify(nativeGetFileIcon.call(app, path, options), cb)
}

const nativeAppMetrics = app.getAppMetrics
app.getAppMetrics = () => {
const metrics = nativeFn.call(app)
const metrics = nativeAppMetrics.call(app)
for (const metric of metrics) {
if ('memory' in metric) {
deprecate.removeProperty(metric, 'memory')
Expand Down
71 changes: 28 additions & 43 deletions spec/api-app-spec.js
Expand Up @@ -769,61 +769,46 @@ describe('app module', () => {
}
})

it('fetches a non-empty icon', done => {
app.getFileIcon(iconPath, (err, icon) => {
expect(err).to.be.null()
expect(icon.isEmpty()).to.be.false()
done()
})
it('fetches a non-empty icon', async () => {
const icon = await app.getFileIcon(iconPath)
expect(icon.isEmpty()).to.be.false()
})

it('fetches normal icon size by default', done => {
app.getFileIcon(iconPath, (err, icon) => {
const size = icon.getSize()
it('fetches normal icon size by default', async () => {
const icon = await app.getFileIcon(iconPath)
const size = icon.getSize()

expect(err).to.be.null()
expect(size.height).to.equal(sizes.normal)
expect(size.width).to.equal(sizes.normal)
done()
})
expect(size.height).to.equal(sizes.normal)
expect(size.width).to.equal(sizes.normal)
})

describe('size option', () => {
it('fetches a small icon', (done) => {
app.getFileIcon(iconPath, { size: 'small' }, (err, icon) => {
const size = icon.getSize()
expect(err).to.be.null()
expect(size.height).to.equal(sizes.small)
expect(size.width).to.equal(sizes.small)
done()
})
it('fetches a small icon', async () => {
const icon = await app.getFileIcon(iconPath, { size: 'small' })
const size = icon.getSize()

expect(size.height).to.equal(sizes.small)
expect(size.width).to.equal(sizes.small)
})

it('fetches a normal icon', (done) => {
app.getFileIcon(iconPath, { size: 'normal' }, (err, icon) => {
const size = icon.getSize()
expect(err).to.be.null()
expect(size.height).to.equal(sizes.normal)
expect(size.width).to.equal(sizes.normal)
done()
})
it('fetches a normal icon', async () => {
const icon = await app.getFileIcon(iconPath, { size: 'normal' })
console.log(icon)
codebytere marked this conversation as resolved.
Show resolved Hide resolved
const size = icon.getSize()

expect(size.height).to.equal(sizes.normal)
expect(size.width).to.equal(sizes.normal)
})

it('fetches a large icon', function (done) {
it('fetches a large icon', async () => {
// macOS does not support large icons
if (process.platform === 'darwin') {
// FIXME(alexeykuzmin): Skip the test.
// this.skip()
return done()
}
if (process.platform === 'darwin') return

app.getFileIcon(iconPath, { size: 'large' }, (err, icon) => {
const size = icon.getSize()
expect(err).to.be.null()
expect(size.height).to.equal(sizes.large)
expect(size.width).to.equal(sizes.large)
done()
})
const icon = await app.getFileIcon(iconPath, { size: 'large' })
const size = icon.getSize()

expect(size.height).to.equal(sizes.large)
expect(size.width).to.equal(sizes.large)
})
})
})
Expand Down