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

8952 option to ignore tray double click events #12496

Merged
merged 6 commits into from May 16, 2018

Conversation

Projects
None yet
6 participants
@mikeykhalil
Contributor

mikeykhalil commented Apr 2, 2018

Fixes #8952. Based off the conversation in the original Issue, it sounded like the agreed upon solution was to allow the application developer to ignore these double click events on macOS. This patch seems to do the trick.

@mikeykhalil mikeykhalil requested review from electron/docs as code owners Apr 2, 2018

@welcome

This comment has been minimized.

welcome bot commented Apr 2, 2018

💖 Thanks for opening this pull request! 💖

typing cat

Here is a list of things that will help get it across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.
    We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@mikeykhalil mikeykhalil changed the title from 8952 ignore tray double click events to 8952 option to ignore tray double click events Apr 11, 2018

* `ignore` Boolean
Sets the option to ignore double click events. Ignoring these events allow you

This comment has been minimized.

@codebytere

codebytere Apr 17, 2018

Member

s/allow/allows

This comment has been minimized.

@mikeykhalil

mikeykhalil Apr 17, 2018

Contributor

D'oh! Good catch. Fixed.

@@ -241,6 +241,15 @@ win.on('hide', () => {
})
```
#### `tray.setIgnoreDoubleClickEvents(ignore)` _macOS_

This comment has been minimized.

@sindresorhus

sindresorhus Apr 17, 2018

Contributor

@codebytere I think this should be a getter/setter instead. It feels more natural in JS and then you can also check whether it's enabled or not.

tray.ignoreDoubleClickEvents = true;

// Somewhere else in the code-base
if (tray.ignoreDoubleClickEvents) {
  //
}

I realize this goes against many of the other Electron APIs, but I would argue it's never to late to improve how the APIs work.

This comment has been minimized.

@mikeykhalil

mikeykhalil Apr 29, 2018

Contributor

This sounds like an interesting proposal. Is this something that we would want done in this PR? Or could we potentially create a follow up PR which converts one of these instance methods to a getter/setter as a proof of concept?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Apr 29, 2018

Member

I realize this goes against many of the other Electron APIs,

Currently the standard is getter and setter methods for All The Things. If people want to change this then we should raise an issue with a firm proposal on what we want changed. For now let's go with setter and getter methods and if we decide in the future to have a new standard on what our apis look like we can come back and address it.

This comment has been minimized.

@sindresorhus

sindresorhus Apr 29, 2018

Contributor

For now let's go with setter and getter methods and if we decide in the future to have a new standard on what our apis look like we can come back and address it.

This PR is missing a getter method.

and if we decide in the future to have a new standard on what our apis look like we can come back and address it.

#12745

This comment has been minimized.

@mikeykhalil

mikeykhalil May 3, 2018

Contributor

Thanks for creating that issue! I just added a getter method method to this PR.

@@ -175,6 +175,10 @@ void Tray::SetHighlightMode(TrayIcon::HighlightMode mode) {
tray_icon_->SetHighlightMode(mode);
}
void Tray::SetIgnoreDoubleClickEvents(bool ignore) {
tray_icon_->SetIgnoreDoubleClickEvents(ignore);

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Apr 17, 2018

Contributor
#if defined(OS_MACOSX)
  tray_icon_->SetIgnoreDoubleClickEvents(ignore);
#endif
@@ -21,6 +21,9 @@ void TrayIcon::SetTitle(const std::string& title) {
void TrayIcon::SetHighlightMode(TrayIcon::HighlightMode mode) {
}
void TrayIcon::SetIgnoreDoubleClickEvents(bool ignore) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Apr 17, 2018

Contributor

There is no need to have an empty implementation here. Please remove it.

This comment has been minimized.

@mikeykhalil

mikeykhalil Apr 29, 2018

Contributor

I believe we need this here or else the linker throws an error. I noticed that the same was done for SetHighlightMode, SetPressedImage, and several other methods. Here is the error that was output when I attempted to build

Undefined symbols for architecture x86_64:
  "atom::TrayIcon::SetIgnoreDoubleClickEvents(bool)", referenced from:
      vtable for atom::TrayIcon in libelectron_lib.a(electron_lib.tray_icon.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

It is certainly possible that I am misunderstanding something though.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Apr 29, 2018

Contributor

In the "atom/browser/ui/tray_icon.h" the method should only be declared on Mac OS:

#if defined(OS_MACOSX)
  // Sets the flag which determines whether to ignore double click events. This
  // only works on macOS.
  virtual void SetIgnoreDoubleClickEvents(bool ignore);
#endif

This comment has been minimized.

@mikeykhalil

mikeykhalil May 3, 2018

Contributor

Updated! I also made these pure virtual methods in order to get rid of the compile time error. This seemed to be consistent with a few other methods in this class that avoided including the empty implementation in tray_icon.cc. Only declaring the methods on Mac would still result with the same error if we didn't make these pure virtual methods.

// If we are ignoring double click events, we should ignore the `clickCount`
// value and immediately emit a click event.
if (ignoreDoubleClickEvents_ == YES) {
trayIcon_->NotifyClicked(

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Apr 17, 2018

Contributor

This shouldn't be simply copypasted.
Please rewrite it with something like that (pseudocode):

shouldBeHandledAsASingleClick = (event.clickCount == 1) || ignoreDoubleClickEvents
if (shouldBeHandledAsASingleClick) {
  trayIcon_->NotifyClicked(...)
}

shouldBeHandledAsADoubleClick = (event.clickCount == 2) && !ignoreDoubleClickEvents
if (shouldBeHandledAsADoubleClick) {
  trayIcon_->NotifyDoubleClicked(...)
}

...

This comment has been minimized.

@mikeykhalil

mikeykhalil Apr 29, 2018

Contributor

Good idea, I like this a lot more. Updated!

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented May 3, 2018

@mikeykhalil Can you please rebase your branch onto the latest electron:master?
It seems like the CI issues are not related to your changes.

@mikeykhalil

This comment has been minimized.

Contributor

mikeykhalil commented May 3, 2018

Rebased, looks like the checks are passing now.

@mikeykhalil

This comment has been minimized.

Contributor

mikeykhalil commented May 7, 2018

Thanks for all of the help and for approving these changes! Let me know if anything else is needed in order to get this merged.

@jkleinsc jkleinsc merged commit 9488ef4 into electron:master May 16, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@welcome

This comment has been minimized.

welcome bot commented May 16, 2018

Congrats on merging your first pull request! 🎉🎉🎉

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