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

Implement app.setBadgeCount on Mac #6309

Merged
merged 5 commits into from Jul 2, 2016

Conversation

Projects
None yet
4 participants
@zcbenz
Contributor

zcbenz commented Jul 1, 2016

This PR implements app.setBadgeCount on Mac and moves APIs under app.launcher to app.

app.launcher is probably not a good name because launcher also means a popular type of programs, and since we are making setBadgeCount work on other platforms, there is no meaning to keep the app.launcher set of APIs.

Also for the isCounterBadgeAvailable API, generally for cross-platform APIs they should usually silently fail on platforms that do not support them, so users don't have to worry about platform differences. In the case of app.setBadgeCount, there is no case that users would care whether the call succeed, so I'm removing it.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jul 1, 2016

Can we make the setBadgeCount API still fail silently but return true or false depending on success / failure. This is consistant with quite a few of the other electron API's.

In regards to why, I know of an instance where an app (closed source) generates a badge count dynamically using BrowserWindow.setIcon() and using a canvas and a buffer to dynamically draw the numbers on an image.

It would be nice to use setBadgeCount but use the Buffer technique as a fall back

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Jul 1, 2016

Can we make the setBadgeCount API still fail silently but return true or false depending on success / failure. This is consistant with quite a few of the other electron API's.

👍

@LinusU

This comment has been minimized.

LinusU commented Jul 1, 2016

Returning false when erroring feels very PHP. It's very easy for consumers to forget to wrap every call in if (!theActuallCall()) throw new Error('...').

If you don't care about a particular error, isn't it better to try/catch?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jul 1, 2016

@LinusU It's what many other Electron methods do and it makes cross platform implementation a lot easier. I shouldn't have to wrap all the partially supported electron API's in try { } catch { } just to stop errors.

I.e. 99.99% of users would end up with code that looks like this

try {
    app.setBadgeCount(10);
} catch (e) {
    // Do nothing...
}

In this use case why would you want to throw an error if the call failed?

@@ -223,9 +225,7 @@ class Browser : public WindowListObserver {
std::string version_override_;
std::string name_override_;
#if defined(OS_LINUX)
int current_badge_count_ = 0;

This comment has been minimized.

@kevinsawicki

kevinsawicki Jul 1, 2016

Contributor

I think this could just be badge_count_, the current_ prefix is kind of redundant.

@@ -12,6 +12,7 @@
#include "base/mac/bundle_locations.h"
#include "base/mac/foundation_util.h"
#include "base/strings/sys_string_conversions.h"
#include "base/strings/string_number_conversions.h"

This comment has been minimized.

@kevinsawicki

kevinsawicki Jul 1, 2016

Contributor

This should go above base/strings/sys_string_conversions.h for alphabetical sorting

})
describe('app.setBadgeCount API', function () {
const shouldFail = process.platform === 'win32' ||
(process.platform === 'linux' && app.isUnityRunning())

This comment has been minimized.

@kevinsawicki

kevinsawicki Jul 1, 2016

Contributor

Should this be !app.isUnityRunning()?

@zcbenz zcbenz merged commit ee0eb9a into master Jul 2, 2016

7 of 8 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-linux-arm Build #3584457 succeeded in 46s
Details
electron-linux-ia32 Build #3584458 succeeded in 40s
Details
electron-linux-x64 Build #3584459 succeeded in 130s
Details
electron-mas-x64 Build #1759 succeeded in 6 min 0 sec
Details
electron-osx-x64 Build #1766 succeeded in 6 min 15 sec
Details
electron-win-ia32 Build #771 succeeded in 6 min 21 sec
Details
electron-win-x64 Build #760 succeeded in 6 min 12 sec
Details

@zcbenz zcbenz deleted the app-launcher-rename branch Jul 2, 2016

@vlad-shatskyi vlad-shatskyi referenced this pull request Jul 6, 2016

Open

Update badge count #561

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