fix: allow Tray with title only (without icon) on Mac #13851

miniak · 2018-07-29T16:18:21Z

Description of Change

Fixes #13202

Checklist

PR description included and stakeholders cc'd
npm test passes
PR title follows semantic commit guidelines

Release Notes

Notes: Allow Tray with title only (without icon) on Mac.

MarshallOfSound · 2018-07-30T00:50:45Z

atom/browser/api/atom_api_tray.cc

+  mate::Handle<NativeImage> image;
+  if (!args->GetNext(&image)) {
+#if !defined(OS_MACOSX)
+    args->ThrowError("'image' must be defined");


An image must be provided as the first argument of the `Tray` constructor

MarshallOfSound · 2018-07-30T00:56:02Z

docs/api/tray.md

@@ -60,9 +60,10 @@ rely on the `click` event and always attach a context menu to the tray icon.

 ### `new Tray(image)`

-* `image` ([NativeImage](native-image.md) | String)
+* `image` ([NativeImage](native-image.md) | String | null)


Although less semantic I'm not a fan of having an argument that can be null on one platform but would throw errors on another. A static type checker (typescript for example) wouldn't be able to warn you about these issues.

Can we instead recommend that uses provide nativeImage.createEmpty() if they wish to have a tray with no icon?

@MarshallOfSound I will investigate that

MarshallOfSound

feedback RE usage semantics and errors

ckerr · 2018-08-11T18:05:38Z

@miniak, @MarshallOfSound, what is the status of this? It's been idle for ~2 wks

MarshallOfSound · 2018-08-11T22:41:59Z

@ckerr pending on my nativeImage.createEmpty() comment

miniak · 2018-08-26T12:49:08Z

@MarshallOfSound you're right, it can easily be done without changing the API by handling native.createEmptyImage() properly
@ckerr please review again, thanks!

alexeykuzmin · 2018-08-28T21:12:30Z

@miniak Can you please resolve conflicts here?

miniak · 2018-08-28T22:22:39Z

@alexeykuzmin done

alexeykuzmin · 2018-08-29T08:43:14Z

@miniak
GYP build was broken at the moment you made the rebase, can you please do it again?

alexeykuzmin

A test maybe?

miniak · 2018-08-29T09:19:42Z

@alexeykuzmin it cannot be tested as it's just a visual change

alexeykuzmin · 2018-08-29T10:03:32Z

spec/api-tray-spec.js

-      tray = new Tray(nativeImage.createEmpty())
-    })
+  beforeEach(() => {
+    tray = new Tray(nativeImage.createEmpty())


Can we have a test on Mac creating a Tray without an icon?

this is how you do it without the icon, it does not accept null

miniak · 2018-08-29T10:08:31Z

@MarshallOfSound can you please unblock the PR?

the reason why the PR was blocked has been addressed, cannot get a response now

miniak · 2018-08-30T06:51:14Z

@alexeykuzmin can we get it merged?

alexeykuzmin · 2018-08-30T09:05:38Z

@alexeykuzmin can we get it merged?

@miniak, I don't have a permission to merge in e/e.
Even if I had it I wouldn't merge anything by other Microsofties =)

release-clerk · 2018-08-30T09:16:59Z

Release Notes Persisted

Allow Tray with title only (without icon) on Mac.

trop · 2018-08-30T09:17:20Z

An error occurred while attempting to backport this PR to "2-0-x",
you will need to perform this backport manually

trop · 2018-08-30T09:17:46Z

We have automatically backported this PR to "3-0-x", please check out #14384

miniak requested review from a team July 29, 2018 16:18

miniak added the target/3-0-x label Jul 29, 2018

miniak force-pushed the miniak/tray-title-only branch 3 times, most recently from 77ef005 to 7169f59 Compare July 29, 2018 16:49

MarshallOfSound added semver/minor backwards-compatible functionality and removed target/3-0-x labels Jul 30, 2018

MarshallOfSound reviewed Jul 30, 2018

View reviewed changes

MarshallOfSound previously requested changes Jul 30, 2018

View reviewed changes

miniak force-pushed the miniak/tray-title-only branch from 7169f59 to 0eb5421 Compare July 30, 2018 09:00

miniak force-pushed the miniak/tray-title-only branch 2 times, most recently from 8798524 to ed0495d Compare August 26, 2018 12:45

miniak changed the title ~~feat: allow Tray with title only (without icon) on Mac~~ fix: allow Tray with title only (without icon) on Mac Aug 26, 2018

miniak added semver/patch backwards-compatible bug fixes target/1-8-x and removed semver/minor backwards-compatible functionality target/1-8-x labels Aug 26, 2018

miniak self-assigned this Aug 27, 2018

miniak force-pushed the miniak/tray-title-only branch from ed0495d to 80b0ce1 Compare August 28, 2018 22:20

alexeykuzmin suggested changes Aug 29, 2018

View reviewed changes

miniak force-pushed the miniak/tray-title-only branch from 80b0ce1 to 124e7c1 Compare August 29, 2018 09:52

alexeykuzmin suggested changes Aug 29, 2018

View reviewed changes

fix: allow Tray with title only (without icon) on Mac

135ab20

miniak force-pushed the miniak/tray-title-only branch from 124e7c1 to 135ab20 Compare August 29, 2018 10:15

alexeykuzmin approved these changes Aug 29, 2018

View reviewed changes

codebytere approved these changes Aug 29, 2018

View reviewed changes

MarshallOfSound approved these changes Aug 30, 2018

View reviewed changes

MarshallOfSound merged commit 5a0770f into master Aug 30, 2018

MarshallOfSound deleted the miniak/tray-title-only branch August 30, 2018 09:17

trop bot mentioned this pull request Aug 30, 2018

fix: allow Tray with title only (without icon) on Mac (backport: 3-0-x) #14384

Merged

trop bot added merged/3-0-x and removed target/3-0-x labels Aug 30, 2018

miniak mentioned this pull request Sep 2, 2018

Tray with only title #5439

Closed

ckerr mentioned this pull request Dec 18, 2018

Draft 4.0.0 Release Notes #16117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow Tray with title only (without icon) on Mac #13851

fix: allow Tray with title only (without icon) on Mac #13851

miniak commented Jul 29, 2018 •

edited

MarshallOfSound Jul 30, 2018 •

edited

MarshallOfSound Jul 30, 2018

miniak Jul 30, 2018

MarshallOfSound left a comment

ckerr commented Aug 11, 2018

MarshallOfSound commented Aug 11, 2018

miniak commented Aug 26, 2018

alexeykuzmin commented Aug 28, 2018

miniak commented Aug 28, 2018

alexeykuzmin commented Aug 29, 2018

alexeykuzmin left a comment

miniak commented Aug 29, 2018

alexeykuzmin Aug 29, 2018

miniak Aug 29, 2018

miniak commented Aug 29, 2018 •

edited

miniak commented Aug 30, 2018

alexeykuzmin commented Aug 30, 2018

release-clerk bot commented Aug 30, 2018

trop bot commented Aug 30, 2018

trop bot commented Aug 30, 2018

fix: allow Tray with title only (without icon) on Mac #13851

fix: allow Tray with title only (without icon) on Mac #13851

Conversation

miniak commented Jul 29, 2018 • edited

Description of Change

Checklist

Release Notes

MarshallOfSound Jul 30, 2018 • edited

Choose a reason for hiding this comment

MarshallOfSound Jul 30, 2018

Choose a reason for hiding this comment

miniak Jul 30, 2018

Choose a reason for hiding this comment

MarshallOfSound left a comment

Choose a reason for hiding this comment

ckerr commented Aug 11, 2018

MarshallOfSound commented Aug 11, 2018

miniak commented Aug 26, 2018

alexeykuzmin commented Aug 28, 2018

miniak commented Aug 28, 2018

alexeykuzmin commented Aug 29, 2018

alexeykuzmin left a comment

Choose a reason for hiding this comment

miniak commented Aug 29, 2018

alexeykuzmin Aug 29, 2018

Choose a reason for hiding this comment

miniak Aug 29, 2018

Choose a reason for hiding this comment

miniak commented Aug 29, 2018 • edited

miniak commented Aug 30, 2018

alexeykuzmin commented Aug 30, 2018

release-clerk bot commented Aug 30, 2018

trop bot commented Aug 30, 2018

trop bot commented Aug 30, 2018

miniak commented Jul 29, 2018 •

edited

MarshallOfSound Jul 30, 2018 •

edited

miniak commented Aug 29, 2018 •

edited