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

Extend the custom Jump List API provided by Electron #6826

Merged
merged 1 commit into from Sep 1, 2016

Conversation

Projects
None yet
4 participants
@enlight
Contributor

enlight commented Aug 12, 2016

Add app.setJumpList(callback) which makes it possible to fully customize the Jump List of an Electron app, specifically it allows to:

  • Add tasks to the standard Tasks category.
  • Add separators to the standard Tasks category.
  • Add custom categories containing tasks and file links.
  • Add system managed Recent/Frequent categories.
  • Remove the custom Jump List.

app.setUserTasks() should probably be deprecated at some point.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Aug 18, 2016

Current API looks inconsistent with other APIs, usually callback is used for event handlers or request callbacks, it is weird to use callback for setting parameter synchronously.

I under understand it is for providing the minItems, removedItems, but since very few developers would really use them, maybe we should make the setJumpList API accept one object parameters, and add another getJumpListSettings API to return minItems, removedItems?

@enlight

This comment has been minimized.

Contributor

enlight commented Aug 18, 2016

Yeah, I agree the API is inconsistent, I'll make the suggested change.

@enlight

This comment has been minimized.

Contributor

enlight commented Aug 19, 2016

Updated the PR to add getJumpListSettings().

@@ -29,6 +29,22 @@ namespace gfx {
class Image;
}
#if defined(OS_WIN)
namespace atom {
enum class JumpListResult : int;

This comment has been minimized.

@zcbenz

zcbenz Aug 24, 2016

Contributor

Since the SetJumpList is a public API of Browser, the JumpListResult should also be public too. So the complete definition of JumpListResult should be placed in browser.h.

namespace mate {
template<>
struct Converter<atom::JumpListResult> {

This comment has been minimized.

@zcbenz

zcbenz Aug 24, 2016

Contributor

The converters for a type should be placed under the _api.cc files that use them, for this one, it should be put in atom_api_app.cc.

@@ -158,6 +174,12 @@ class Browser : public WindowListObserver {
// Add a custom task to jump list.
bool SetUserTasks(const std::vector<UserTask>& tasks);
// Get the current Jump List settings.
v8::Local<v8::Value> GetJumpListSettings(mate::Arguments* args);

This comment has been minimized.

@zcbenz

zcbenz Aug 24, 2016

Contributor

The APIs under atom::Browser are also meant to be used by other parts of Electron code apart from V8 bindings, so methods under this class should not have dependency of V8. If an API is only exposed in JavaScript, it should be put in the api:: classes directly. This is for the good of having a clean architecture.

So we should either make Browser::GetJumpListSettings and Browser::SetJumpList independent from V8, or just put them under api::App. And the browser.cc should not include any V8 code.

A few places are violating this rule and escaped review, and we will fix them.

This comment has been minimized.

@enlight

enlight Aug 24, 2016

Contributor

Ah OK, I wasn't clear on the distinction between Browser and App, thanks for explaining it.

@enlight

This comment has been minimized.

Contributor

enlight commented Aug 25, 2016

Rebased on master and moved setJumpList and getJumpListSettings to atom_app_api.h/cc since they're mostly just glue code. I've left setUserTasks in Browser for now, if you want it moved to App I can submit a separate PR.

@enlight

This comment has been minimized.

Contributor

enlight commented Aug 26, 2016

Hmm, no idea why those builds on Travis failed.

@@ -523,6 +736,63 @@ void App::OnCertificateManagerModelCreated(
}
#endif
#if defined(OS_WIN)
v8::Local<v8::Value> App::GetJumpListSettings(mate::Arguments* args) {

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Since the code is not using members of mate::Arguments, this parameter can be omitted and the isolate can be got from isolate() member.

The method can also return mate::Dictionary directly.

This comment has been minimized.

@enlight

enlight Aug 29, 2016

Contributor

Returning mate::Dictionary directly would be inconsistent with the rest of Electron's bindings code.

@@ -24,9 +24,21 @@ namespace base {
class FilePath;
}
namespace atom {
enum class JumpListResult : int;

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

This can be moved to inside namespace atom below.

#if defined(OS_WIN)
template<>
struct Converter<atom::JumpListResult> {

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

This declaration seems redundant.

DCHECK(shell_item);
DCHECK(file_name);
wchar_t* file_name_buffer = nullptr;

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

There is base::win::ScopedCoMem to avoid manual memory freeing.

item->type = JumpListItem::Type::TASK;
wchar_t path[MAX_PATH];
constexpr size_t max_path = std::extent<decltype(path)>::value;

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

While this is nice, in this simple case using the arraysize macro would make the code more easy to read, arraysize is used very widely in Chromium so everyone knows what arraysize means, and it is shorter to use.

base::win::ScopedPropVariant prop;
if (SUCCEEDED(property_store->GetValue(PKEY_Link_Arguments, prop.Receive()))
&& (prop.get().vt == VT_LPWSTR)) {
item->arguments = base::string16(prop.get().pwszVal);

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

I didn't test but the base::string16 wrapper seems unnecessary.

&& (prop.get().vt == VT_LPWSTR)) {
item->arguments = base::string16(prop.get().pwszVal);
} else {
item->arguments.clear();

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

The clear calls in this function are redundant, since the members of item are empty or assigned to 0 by default.

IShellItem* shell_item;
IShellLink* shell_link;
for (UINT i = 0; i < removed_count; ++i) {
if (SUCCEEDED(in->GetAt(i, IID_PPV_ARGS(&shell_item)))) {

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

The standard way for this in Electron is to protect the item with base::win::ScopedComPtr, so there is no manual release needed.

base::win::ScopedComPtr<IShellItem> shell_item;
in->GetAt(i, IID_PPV_ARGS(shell_item.Receive()));

This comment has been minimized.

@enlight

enlight Aug 29, 2016

Contributor

The same could be done with CComPtr so I don't see the need to use base::win::ScopedComPtr here unless you want me to replace all the other CComPtrs in this file. I didn't use CComPtr in this case because the loop is very simple and constructing two CComPtrs every iteration didn't seem like it was worth the effort.

namespace atom {
bool JumpList::CreateDestinationList() {
return SUCCEEDED(destinations_.CoCreateInstance(CLSID_DestinationList));

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Putting this call in constructor can make code simpler.

@@ -527,6 +527,151 @@ Adds `tasks` to the [Tasks][tasks] category of the JumpList on Windows.
Returns `true` when the call succeeded, otherwise returns `false`.
**Note:** If you'd like to customize the Jump List even more use
`app.setJumpList(callback)` instead.

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

This should be updated.

* `name` String - Must be set if `type` is `"custom"`, otherwise it should be
omitted.
* `items` Array - Array of `JumpListItem` objects if `type` is `"tasks"` or
`"custom"`, otherwise it should be omitted.

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

This line is not aligned.

* `program` String - Path of the program to execute, usually you should
specify `process.execPath` which opens the current program. Should only be
set if `type` is `"task"`.
* `arguments` String - The command line arguments when `program` is

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Can you rename this to args? It was a wrong decision to use arguments since it is a keyword of JavaScript and all other places are using args, it is good a chance to fix this in the new API.

@enlight

This comment has been minimized.

Contributor

enlight commented Aug 31, 2016

OK, I've made all the changes you've requested, except where you left a 👍 on my comments (which I assume means you agree with me on those points). Here's the list of changes since the previous version:

  • Remove redundant forward declaration of Converter<atom::JumpListResult>.
  • Remove mate::Arguments parameter from App::GetJumpListSettings().
  • Use base::win::ScopedCoMem in GetShellItemFileName().
  • Replace std::extent<decltype(path)>::value with the more readable arraysize(path).
  • Remove redundant calls to base::string16 in ConvertShellLinkToJumpListItem().
  • Remove redundant calls to clear() in the JumpListItem mate converter.
  • Merge JumpList::CreateDestinationList() into the JumpList constructor.
  • Rename the arguments property of the Jump List item object to args in the public API since the former is a JS keyword.
  • Clean up the docs.

P.S. Unfortunately GitHub doesn't send me notifications when you leave a 👍, so next time, make some noise when you review :)

Jump List (for a more detailed description of this value see the
[MSDN docs][JumpListBeginListMSDN]).
* `removedItems` Array - Array of `JumpListItem` objects that correspond to
items that the user has explicitely removed from custom categories in the

This comment has been minimized.

@zeke

zeke Aug 31, 2016

Member

explicitely -> explicitly

Sets or removes a custom Jump List for the application, and returns one of the
following strings:
* `"ok"` - Nothing went wrong.

This comment has been minimized.

@zeke

zeke Aug 31, 2016

Member

I would suggest dropping the doublequotes on these strings for cleanliness. It's clear enough that they are strings.

`JumpListCategory` objects should have the following properties:
* `type` String - One of the following:
* `"tasks"` - Items in this category will be placed into the standard `Tasks`

This comment has been minimized.

@zeke

zeke Aug 31, 2016

Member

Same here. No quotes.

Extend the custom Jump List API
Add `app.getJumpListSettings()` and `app.setJumpList(callback)` that
make it possible to fully customize the Jump List of an Electron app.
It is now possible to:
- Add tasks to the standard `Tasks` category.
- Add separators to the standard `Tasks` category.
- Add custom categories containing tasks and file links.
- Add system managed Recent/Frequent categories.
- Remove the custom Jump List.
@enlight

This comment has been minimized.

Contributor

enlight commented Sep 1, 2016

Updated the docs as suggested by @zeke

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Sep 1, 2016

Thanks!

@zcbenz zcbenz merged commit 14f625d into electron:master Sep 1, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bpasero

This comment has been minimized.

Contributor

bpasero commented Sep 17, 2016

@enlight thanks for this API! If I am calling into setJumpList is it still possible to add recent files via app.addRecentDocument?

@bpasero

This comment has been minimized.

Contributor

bpasero commented Sep 17, 2016

@enlight actually now I see the docs and there is a category recent and frequent. I assume the category recent is needed so that app.addRecentDocument works.

Does windows fill the frequent entries automatically based on usage of files?

@enlight

This comment has been minimized.

Contributor

enlight commented Sep 17, 2016

@bpasero Yes, files added via app.addRecentDocument should show up in the recent category, I believe files selected via dialog.showOpenDialog will also end up in that list. The frequent category is populated in the same way. Here's the relevant text from the docs:

The contents of the Frequent and Recent categories are calculated for each application that uses SHAddToRecentDocs directly. In some cases of user action, such as opening a file through Windows Explorer or using the common file dialog box to open, save, or create a file, the Shell calls SHAddToRecentDocs on behalf of an application and those calls are also taken into account in the usage statistics. The Shell also calls SHAddToRecentDocs on behalf of the application when a destination is launched from its Jump List. However, it is good practice for the application to explicitly call SHAddToRecentDocs itself even if it is expected that the Shell will make the call. This guarantees that the usage is recorded, and the algorithms for tracking recent or frequent usage will correct for any duplicate calls.

@bpasero

This comment has been minimized.

Contributor

bpasero commented Sep 17, 2016

@enlight great. one thing puzzles me though: we (VS Code) never could get the entries we added via app.addRecentDocument to show up in the task bar. Now, when I use the new setJumpList and add an empty category recent, they start to show up! This was driving me crazy and I spent hours over hours to find the bug on our end by checking if some Windows registry key was wrong or missing...

One thing I noticed is that the issue was caused by us calling app.setUserTasks. With the new setJumpList I am not using that call anymore and if I take it out completely (and also do not call app.setJumpList, the recent file entries start to show up.

Anyhow, now with setJumpList it seems to work just fine 👍

@enlight

This comment has been minimized.

Contributor

enlight commented Sep 17, 2016

@bpasero The default Jump List displays the Recent category, but a custom Jump List completely replaces the default one (rather than amending it), so all the categories you want to show up must be added explicitly. app.setUserTasks creates a custom Jump List that only contains the Tasks category, which is why there is no Recent category displayed in the Jump List after you call that function.

Anyway, happy to hear you now have one less thing to worry about :)

@bpasero

This comment has been minimized.

Contributor

bpasero commented Sep 18, 2016

@enlight that explains everything :). I think it should be documented though 👍

The only puzzling thing left for me is that app.addRecentDocument does not allow to add folders to the list. The windows explorer also shows folders for recent documents, but when I add them, they never show up.

@bpasero

This comment has been minimized.

Contributor

bpasero commented Nov 7, 2016

@enlight have you heard about an issue where the custom entries of the jump list are all gone once you do this:

image

As soon as I pick "Remove from this list" on my custom category "Recent Folders", the custom category is gone for good. No restart helps. The only thing that does help is a complete reinstall :-(

@enlight

This comment has been minimized.

Contributor

enlight commented Nov 8, 2016

@bpasero Did a search through the VSCode repo and didn't see app.getJumpListSettings() called anywhere, so...

When a user unpins an item from the Jump List Windows will keep track of it, if you then attempt to add that item again with app.setJumpList() Windows will simply throw out the whole custom category the item belongs to.

To fix this in VSCode you need to call app.getJumpListSetting() to obtain the removedItems list before calling app.setJumpList(), and ensure that none of the items in removedItems are added back into the Jump List. Note that this doesn't mean that once removed an item can never be added back into the Jump List, Windows will clear the removedItems list after a successful call to app.setJumpList(), at which point you can call app.setJumpList() again to add the previously removed item back in.

@bpasero

This comment has been minimized.

Contributor

bpasero commented Nov 8, 2016

@enlight thanks man, I was not aware of this method. will give it a try!!

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