GH-5706: DIsposed the clipboard copy listener. #5709
Conversation
| disposable = addClipboardListener(document.documentElement, 'copy', e => this.handleCopy(e, downloadUrl)); | ||
| } finally { | ||
| if (disposable) { | ||
| disposable.dispose(); |
There was a problem hiding this comment.
it should be disposed within event handler before calling this.handleCopy.
There was a problem hiding this comment.
Can you explain why?
There was a problem hiding this comment.
because we want to wait till copy event is fired and remove a listener only after it? I don't understand how current change ensures it, i.e. it removes a listener before calling copy.
We use the same pattern in other places as well: https://github.com/theia-ide/theia/blob/60dcb3326ea5e70c8d1017dcd206013304a5c92e/packages/markers/src/browser/problem/problem-contribution.ts#L158-L168
| if (document.documentElement) { | ||
| addClipboardListener(document.documentElement, 'copy', e => this.handleCopy(e, downloadUrl)); | ||
| try { | ||
| disposable = addClipboardListener(document.documentElement, 'copy', e => this.handleCopy(e, downloadUrl)); |
There was a problem hiding this comment.
const toDispose = addClipboardListener(document.documentElement, 'copy', e => {
toDispose.dispose();
this.handleCopy(e, downloadUrl);
});
| @@ -22,6 +22,7 @@ import { FileSystem } from '../../common/filesystem'; | |||
| import { FileDownloadData } from '../../common/download/file-download-data'; | |||
| import { MessageService } from '@theia/core/lib/common/message-service'; | |||
| import { addClipboardListener } from '@theia/core/lib/browser/widgets'; | |||
| import { Disposable } from '@theia/core/src/common/disposable'; | |||
Closes #5706. Signed-off-by: Akos Kitta <[email protected]>
|
I am OK merging this, but then the static download link is gone. |
|
@kittaakos let's merge it and deal with #5707 separately? |
Closes #5706.
Signed-off-by: Akos Kitta [email protected]
Related: #5708.