Add the ability to disable/enable the context menu #93

loopmode · 2020-02-01T13:27:22Z

This PR adds a dispose function and fixes #82 and fixes #84.

The implementation is such that the contextMenu() return value is a function that will remove the registered event listeners.

loopmode · 2020-02-01T13:31:33Z

@sindresorhus I also have updated the fixture.js and html files, but I'm not sure whether I did it "correctly", so it's parked on a separate branch. See loopmode@26f36c4

It adds a checkbox to enable or disable the context menu, but for that to work while keeping it simple, I moved the entire logic our of the js and into the html file.

I can provide another PR for that.

index.js

sindresorhus · 2020-02-12T20:45:53Z

index.js

+
+	const dispose = () => {
+		disposables.forEach(dispose => dispose());
+		isDisposed = true;


Instead of a boolean, just empty the array and check the length. You should empty the array regardless as you don't want to hold onto those forever.

Hmm. I believe it would cause a chicken/egg problem and we'd never create the menu in the first place in https://github.com/loopmode/electron-context-menu/blob/feature/dispose-function/index.js#L258-L265

Looking into it.

Indeed, it's not that easy.
Avoiding the flag requires either a completely different approach, or several more changes that seem unnecessarily complex.
It's really about bailing out in certain deferred edge cases, and the significant piece of information of the flag is: "No need to create the menu anymore, the callsite already disposed it".

I already tried a couple variants, and I still believe the flag is the best solution.

Should let go of the references despite of that.

array length reset

sindresorhus · 2020-02-12T20:47:02Z

This looks great :)

You also need to add it to https://github.com/sindresorhus/electron-context-menu/blob/master/index.d.ts

sindresorhus · 2020-02-12T20:48:34Z

It adds a checkbox to enable or disable the context menu, but for that to work while keeping it simple, I moved the entire logic our of the js and into the html file.

I think you should create a new fixture just for this. It's totally fine that it's in the HTML file. It can be as just contextMenu(), with no options. It should be part of this PR.

loopmode · 2020-02-13T06:21:29Z

Great, will do that!

sindresorhus · 2020-04-14T07:08:47Z

Bump :)

# Conflicts: # package.json

# Conflicts: # index.js

loopmode · 2020-04-15T09:49:39Z

Hey @sindresorhus

So.. I did quite some things extra here, hope it doesn't make accepting the PR too much of a problem?

I merged your 1.0 from master
I added a .prettierrc which basically resembles the existing code style, at least I was able to format on save without causing extra changes
I added a fixture for creating and disposing the menu, it can be used via yarn start-fixture3. I suggest you/we rename the fixture scripts :)
Because the fixture files started to dominate the project root, i moved all of them into a fixtures/ subfolder and adjusted the package scripts accordingly
The array of disposable functions does get emptied on dispose now, but I still did leave the isDisposed flag. As mentioned previously, it's currently essential. Maybe it's my lack of imagination, but I don't see another simple or elegant approach. Emptying the array and checking for length === 0 is unfortunately definitely not the way to go, as it cannot distinguish between "already disposed" and "not initialized yet". I'd be happy to learn something here, but really I think the boolean flag is the way to go

Ah, and I added couple of cleanup commits to pass through your travis pipeline.

sindresorhus · 2020-04-22T15:14:34Z

I added a .prettierrc which basically resembles the existing code style, at least I was able to format on save without causing extra changes

I'm not interested in having this.

sindresorhus · 2020-04-22T15:14:40Z

I added a fixture for creating and disposing the menu, it can be used via yarn start-fixture3. I suggest you/we rename the fixture scripts :)
Because the fixture files started to dominate the project root, i moved all of them into a fixtures/ subfolder and adjusted the package scripts accordingly
The array of disposable functions does get emptied on dispose now, but I still did leave the isDisposed flag. As mentioned previously, it's currently essential. Maybe it's my lack of imagination, but I don't see another simple or elegant approach. Emptying the array and checking for length === 0 is unfortunately definitely not the way to go, as it cannot distinguish between "already disposed" and "not initialized yet". I'd be happy to learn something here, but really I think the boolean flag is the way to go

👍

sindresorhus · 2020-04-22T15:07:12Z

readme.md

@@ -52,10 +52,21 @@ let mainWindow;
 })();
 ```

+Note that the return value of `contextMenu()` is a dispose function:


The readme and index.d.ts should be in sync documentation-wise.

There are a couple of differences, as I didn't know about the sync aspect. Will adjust.
Actually, there's even a react hook example in index.d.ts that is not in readme.
I will remove that altogether as it feels out of scope here.

The readme and index.d.ts should be in sync documentation-wise.

Synced

sindresorhus · 2020-04-22T15:07:34Z

index.js

+				label: 'No Guesses Found',
+				visible: hasText && props.misspelledWord,
+				enabled: false
+			});


Don't do unrelated changes.

Yeah, sorry. Related to prettierrc - will disable auto-formatting in my editor.

Don't do unrelated changes.

Reverted

loopmode · 2020-04-23T06:47:31Z

I added a .prettierrc which basically resembles the existing code style, at least I was able to format on save without causing extra changes

I'm not interested in having this.

removed

The readme and index.d.ts should be in sync documentation-wise.

Synced

Don't do unrelated changes.

Reverted

sindresorhus · 2020-04-27T10:35:59Z

package.json

@@ -10,10 +10,12 @@
 		"email": "sindresorhus@gmail.com",
 		"url": "https://sindresorhus.com"
 	},
+	"contributors": ["Jovica Aleksic <jovica.aleksic@loopmode.de>"],


I don't use this field. It's not actually used anywhere and it's creates inequality with contributors that doesn't add themselves there. I prefer to keep it simple and just not use it. Hope you understand :)

Absolutely no problem! :)

sindresorhus · 2020-04-27T10:38:26Z

Thanks :)

etecture-jaleksic and others added 2 commits October 20, 2019 12:18

return dispose function

fec694d

Return a dispose function

16f4aa2

Jovica Aleksic added 2 commits February 1, 2020 14:35

cleanup - remove ava warnings

75fa344

rename variable

731bd5b

sindresorhus requested changes Feb 12, 2020

View reviewed changes

etecture-jaleksic added 12 commits April 15, 2020 06:38

Merge branch 'master' of github.com:loopmode/electron-context-menu

0f99066

# Conflicts: # package.json

return named function

cbe729a

fix reference for removeEventListener

75f1e19

rename for readability, formatting

01ff982

move fixture into subfolder

79fcb58

add fixture for dispose functionality

6a7dc2e

Merge remote-tracking branch 'remotes/upstream/master'

54de50b

# Conflicts: # package.json

Merge branch 'master' into feature/dispose-function

942c67d

# Conflicts: # index.js

clear references upon dispose

807e671

formatting, semicolon

eb1a65f

formatting, blank lines

443bc0f

adjust test for returned dispose function

1086202

loopmode requested a review from sindresorhus April 15, 2020 10:25

sindresorhus requested changes Apr 22, 2020

View reviewed changes

etecture-jaleksic added 3 commits April 23, 2020 08:33

cleanup - remove .prettierrc

ad470fd

docs: adjust wording, drop react hook example, sync readme and jsdoc

1d62466

revert formatting

46fa0f8

etecture-jaleksic added 2 commits April 23, 2020 09:05

Merge remote-tracking branch 'remotes/upstream/master'

a8703cb

Merge branch 'master' into feature/dispose-function

ddcf744

loopmode requested a review from sindresorhus April 23, 2020 07:09

yourfavorite mentioned this pull request Apr 26, 2020

Different items depending the element clicked #49

Closed

sindresorhus added 4 commits April 27, 2020 18:23

Update index.d.ts

c6fc2c8

Update readme.md

74d3f3c

Update fixture-toggle.html

6088836

Update index.js

3dfd062

sindresorhus reviewed Apr 27, 2020

View reviewed changes

Update package.json

28014e2

sindresorhus changed the title ~~Return a dispose function~~ Add the ability to disable/enable the context menu Apr 27, 2020

sindresorhus merged commit 2547936 into sindresorhus:master Apr 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to disable/enable the context menu #93

Add the ability to disable/enable the context menu #93

loopmode commented Feb 1, 2020 •

edited by sindresorhus

loopmode commented Feb 1, 2020

sindresorhus Feb 12, 2020

loopmode Apr 15, 2020 •

edited

loopmode Apr 15, 2020 •

edited

loopmode Apr 23, 2020

sindresorhus commented Feb 12, 2020

sindresorhus commented Feb 12, 2020

loopmode commented Feb 13, 2020

sindresorhus commented Apr 14, 2020

loopmode commented Apr 15, 2020 •

edited

sindresorhus commented Apr 22, 2020

sindresorhus commented Apr 22, 2020

sindresorhus Apr 22, 2020

loopmode Apr 23, 2020

loopmode Apr 23, 2020

sindresorhus Apr 22, 2020

loopmode Apr 23, 2020

loopmode Apr 23, 2020

loopmode commented Apr 23, 2020 •

edited

sindresorhus Apr 27, 2020

loopmode Apr 27, 2020

sindresorhus commented Apr 27, 2020

Add the ability to disable/enable the context menu #93

Add the ability to disable/enable the context menu #93

Conversation

loopmode commented Feb 1, 2020 • edited by sindresorhus

loopmode commented Feb 1, 2020

Choose a reason for hiding this comment

loopmode Apr 15, 2020 • edited

Choose a reason for hiding this comment

loopmode Apr 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Feb 12, 2020

sindresorhus commented Feb 12, 2020

loopmode commented Feb 13, 2020

sindresorhus commented Apr 14, 2020

loopmode commented Apr 15, 2020 • edited

sindresorhus commented Apr 22, 2020

sindresorhus commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loopmode commented Apr 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Apr 27, 2020

loopmode commented Feb 1, 2020 •

edited by sindresorhus

loopmode Apr 15, 2020 •

edited

loopmode Apr 15, 2020 •

edited

loopmode commented Apr 15, 2020 •

edited

loopmode commented Apr 23, 2020 •

edited