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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ubuntu Unity launcher counter badge #6243

Merged
merged 7 commits into from Jun 29, 2016

Conversation

Projects
None yet
5 participants
@jnugh
Contributor

jnugh commented Jun 26, 2016

Hi,
I implemented a way to change the unity launcher counter badge from within electron:
Screenshot

For this to work the app needs to be started with env CHROME_DESKTOP=[name of desktop file] or (which I found out about while writing this: set desktopName in package.json the way you would do for making the progressbar work).

Please let me know what you think. When there is something to improve, just let me know. 馃懛

@paulcbetts

This comment has been minimized.

Contributor

paulcbetts commented Jun 26, 2016

Maybe assume that the package name is /usr/share/applications/PACKAGE_ID.desktop if it's unset?

@@ -45,6 +45,13 @@ if (process.platform === 'darwin') {
}
}
if (process.platform === 'linux' && bindings.unityLauncherAvailable()) {

This comment has been minimized.

@zcbenz

zcbenz Jun 26, 2016

Contributor

The existence of API should not depend on current environment, if an API is available on one Linux distribution, it should be able to be called on other Linux distributions, even though it may fail.

We can add a app.unityLauncher.isAvailable API to let user decide how to behave on different platforms.

@@ -647,6 +647,17 @@ Sets the application's [dock menu][dock-menu].
Sets the `image` associated with this dock icon.
### `app.unityLauncher.setBadgeCount(count)` _Ubuntu Unity_

This comment has been minimized.

@zcbenz

zcbenz Jun 26, 2016

Contributor

Ubuntu Unity is not a platform, we should say _Linux_, and make it clear in the docs that it only works for Unity launcher.

This comment has been minimized.

@jnugh

jnugh Jun 26, 2016

Contributor

In this case app.unityLauncher should be renamed, so that this api could work for other desktop environments that support a counter badge (Not sure if there are any 馃槈).
My first attempt was to expose app.dock.setBadgeCount which I removed because there might be code like this out there:

if(app.dock !== undefined) { //Assume we are on MacOS
  app.dock.setBadgeText("Hello World"); //:bomb:
}
@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Jun 26, 2016

Maybe assume that the package name is /usr/share/applications/PACKAGE_ID.desktop if it's unset?

That's our current behavior, but it is only working for packaged app for now.

@@ -216,6 +223,8 @@ class Browser : public WindowListObserver {
std::string version_override_;
std::string name_override_;
int current_badge_count_ = 0;

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 28, 2016

Contributor

Should this go behind a #if defined(OS_LINUX) check since those methods are behind that check?

This comment has been minimized.

@jnugh

jnugh Jun 28, 2016

Contributor

I guess the compiler would remove this as unused, because it is private.
But I will add an ifdef block, to keep the code clean.

**Note:** As `setBadgeCount` only supports Ubuntu Unity, the value will be 0 when the application is running in a different environment.
### `app.launcher.isCounterBadgeAvailable()` _Linux_

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 28, 2016

Contributor

Since this returns the value of unity::isRunning() maybe this method should just be called app.launcher.isUnityRunning() so it becomes a general purpose check people can use to know when the launcher is available.

This comment has been minimized.

@jnugh

jnugh Jun 28, 2016

Contributor

My intention was to make this method as general as possible, to allow an implementation for a different OS, while using the same JS API.
I could add a function to check for unity additionally to this, but would propose to keep a general one as well.

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 28, 2016

Contributor

Good point 馃憤

**Note:** you need to specify the .desktop file name to the desktopName field in package.json. By default, it will assume app.getName().desktop in packaged apps.
### `app.launcher.getBadgeCount(count)` _Linux_

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 28, 2016

Contributor

This method takes no arguments right? Looks like it takes a count right now from the docs.

* `count` Integer
Sets the number to be displayed next to the app icon in the unity launcher.
Setting count to `0` will hide the badge.

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 28, 2016

Contributor

Maybe Setting the count instead of Setting count?

Sets the number to be displayed next to the app icon in the unity launcher.
Setting count to `0` will hide the badge.
**Note:** This feature is currently only supported on Ubuntu Unity. Calling this function has no effect, when the application is running in a different environment.

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 28, 2016

Contributor

Comma not needed after effect

@@ -45,6 +45,14 @@ if (process.platform === 'darwin') {
}
}
if (process.platform === 'linux') {
app.launcher = {

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 28, 2016

Contributor

Would you mind adding some spec coverage for these new APIs?

You could add them after this line:

Not sure if Unity is running on our build servers or not but at least we could cover that calling these methods does not crash the app and they return data of expected types.

### `app.launcher.setBadgeCount(count)` _Linux_
* `count` Integer
Sets the number to be displayed next to the app icon in the unity launcher.

This comment has been minimized.

@sindresorhus

sindresorhus Jun 29, 2016

Contributor

unity => Unity

**Note:** This feature is currently only supported on Ubuntu Unity. Calling this function has no effect, when the application is running in a different environment.
**Note:** you need to specify the .desktop file name to the desktopName field in package.json. By default, it will assume app.getName().desktop in packaged apps.

This comment has been minimized.

@sindresorhus

sindresorhus Jun 29, 2016

Contributor

you => You

This comment has been minimized.

@sindresorhus

sindresorhus Jun 29, 2016

Contributor

Use

`

around desktopName and app.getName().desktop

@paulcbetts

This comment has been minimized.

Contributor

paulcbetts commented Jun 29, 2016

@jnugh This is a great PR! Looking forward to it landing, we'll definitely be using this in the Slack app on Linux

describe('app.launcher API', function() {
it('should be available on linux', function() {
if (process.platform !== 'linux') {
assert.equal(app.launcher, undefined);

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 29, 2016

Contributor

There was previously an issue where npm run lint did not properly lint the spec/ folder.

This was fixed in #6289 so if you rebase or merge master into this branch and then run npm run lint, you should see some errors for this file like the ; at the end of this line.

Sorry that this wasn't picked up sooner.

This comment has been minimized.

@jnugh

jnugh Jun 29, 2016

Contributor

Actually I didn't remember to run the linter...
Rebased, Fixed the errors, squashed it into the specs commit 馃槃

@kevinsawicki kevinsawicki merged commit 6d7b52e into electron:master Jun 29, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 29, 2016

Awesome, thanks for this @jnugh 馃憤

This will be really nice to have on Ubuntu and great job on the docs and tests.

saenzramiro added a commit to ramboxapp/community-edition that referenced this pull request Jul 13, 2016

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