Skip to content
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

[preferences] prevent closing preference editor with the middle mouse click #6198

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

vince-fugnitto
Copy link
Member

What it does

Fixes #2639

  • prevent closing preference editors using the middle mouse click by
    setting the title.closable to false.

How to test

  1. open the preferences widget (F1 > Settings: Open Preferences)
  2. attempt to close the User preference editor using the middle-mouse click
    (the editor should not be closable)
  3. close the preferences widget, the User tab should properly close

Review checklist

Reminder for reviewers

Signed-off-by: Vincent Fugnitto vincent.fugnitto@ericsson.com

Fixes #2639

- prevent closing preference editors using the middle mouse click by
setting the `title.closable` to `false`.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto vince-fugnitto added bug bugs found in the application preferences issues related to preferences labels Sep 16, 2019
@vince-fugnitto vince-fugnitto self-assigned this Sep 16, 2019
@elaihau
Copy link
Contributor

elaihau commented Sep 16, 2019

I am getting the following errors when testing the changes:

bundle.js:113866 root ERROR onWillSave listeners should provide edits, not directly alter the document.
log @ bundle.js:113866
/vs/editor/editor.main.js:64777 Uncaught Error: Model is disposed!
    at TextModel._assertNotDisposed (/vs/editor/editor.main.js:64777)
    at TextModel.getLineCount (/vs/editor/editor.main.js:65077)
    at MonacoEditorModel.get [as lineCount] (/25.bundle.js:250)
    at Function.documentContentLines (/82.bundle.js:2421)
    at DirtyDiffModel.push.../../packages/git/lib/browser/dirty-diff/dirty-diff-manager.js.DirtyDiffModel.handleDocumentChanged (/82.bundle.js:2308)
    at /82.bundle.js:2094
    at invokeFunc (bundle.js:45754)
    at trailingEdge (bundle.js:45801)
    at timerExpired (bundle.js:45789)
logger-protocol.ts:112 root ERROR onWillSave listeners should provide edits, not directly alter the document.
log @ logger-protocol.ts:112
(anonymous) @ logger-frontend-module.ts:41
(anonymous) @ logger.ts:312
(anonymous) @ logger.ts:304
Promise.then (async)
../../packages/core/lib/common/logger.js.Logger.log @ logger.ts:299
log @ logger.ts:45
r @ backend.js:1
(anonymous) @ monaco-editor-model.js:534
step @ monaco-editor-model.js:58
(anonymous) @ monaco-editor-model.js:39
fulfilled @ monaco-editor-model.js:30
Promise.then (async)
step @ monaco-editor-model.js:32
(anonymous) @ monaco-editor-model.js:33
push.../../packages/monaco/lib/browser/monaco-editor-model.js.__awaiter @ monaco-editor-model.js:29
(anonymous) @ monaco-editor-model.js:494
(anonymous) @ event.ts:232
step @ event.ts:15
(anonymous) @ event.ts:15
fulfilled @ event.ts:15
Promise.then (async)
step @ event.ts:15
(anonymous) @ event.ts:15
../../packages/core/lib/common/event.js.__awaiter @ event.ts:15
../../packages/core/lib/common/event.js.Emitter.sequence @ event.ts:229
(anonymous) @ monaco-editor-model.js:494
step @ monaco-editor-model.js:58
(anonymous) @ monaco-editor-model.js:39
(anonymous) @ monaco-editor-model.js:33
push.../../packages/monaco/lib/browser/monaco-editor-model.js.__awaiter @ monaco-editor-model.js:29
push.../../packages/monaco/lib/browser/monaco-editor-model.js.MonacoEditorModel.fireWillSaveModel @ monaco-editor-model.js:488
(anonymous) @ monaco-editor-model.js:463
step @ monaco-editor-model.js:58
(anonymous) @ monaco-editor-model.js:39
(anonymous) @ monaco-editor-model.js:33
push.../../packages/monaco/lib/browser/monaco-editor-model.js.__awaiter @ monaco-editor-model.js:29
push.../../packages/monaco/lib/browser/monaco-editor-model.js.MonacoEditorModel.doSave @ monaco-editor-model.js:455
(anonymous) @ monaco-editor-model.js:404
(anonymous) @ monaco-editor-model.js:298
step @ monaco-editor-model.js:58
(anonymous) @ monaco-editor-model.js:39
(anonymous) @ monaco-editor-model.js:33
push.../../packages/monaco/lib/browser/monaco-editor-model.js.__awaiter @ monaco-editor-model.js:29
(anonymous) @ monaco-editor-model.js:292
Promise.then (async)
push.../../packages/monaco/lib/browser/monaco-editor-model.js.MonacoEditorModel.run @ monaco-editor-model.js:292
push.../../packages/monaco/lib/browser/monaco-editor-model.js.MonacoEditorModel.scheduleSave @ monaco-editor-model.js:404
push.../../packages/monaco/lib/browser/monaco-editor-model.js.MonacoEditorModel.save @ monaco-editor-model.js:288
(anonymous) @ preferences-tree-widget.ts:394
step @ preferences-tree-widget.ts:15
(anonymous) @ preferences-tree-widget.ts:15
(anonymous) @ preferences-tree-widget.ts:15
push.../../packages/preferences/lib/browser/preferences-tree-widget.js.__awaiter @ preferences-tree-widget.ts:15
foldersPreferences_1.title.clickableTextCallback @ preferences-tree-widget.ts:393
execute @ preference-editor-widget.ts:106
../../node_modules/@phosphor/commands/lib/index.js.CommandRegistry.execute @ index.js:351
../../node_modules/@phosphor/widgets/lib/menu.js.Menu.triggerActiveItem @ menu.js:305
../../node_modules/@phosphor/widgets/lib/menu.js.Menu._evtMouseUp @ menu.js:641
../../node_modules/@phosphor/widgets/lib/menu.js.Menu.handleEvent @ menu.js:451
textModel.ts:372 Uncaught Error: Model is disposed!
    at TextModel._assertNotDisposed (textModel.ts:372)
    at TextModel.getLineCount (textModel.ts:745)
    at MonacoEditorModel.get [as lineCount] (monaco-editor-model.js:240)
    at Function.documentContentLines (dirty-diff-manager.ts:279)
    at DirtyDiffModel.push.../../packages/git/lib/browser/dirty-diff/dirty-diff-manager.js.DirtyDiffModel.handleDocumentChanged (dirty-diff-manager.ts:205)
    at dirty-diff-manager.ts:78
    at invokeFunc (index.js:160)
    at trailingEdge (index.js:207)
    at timerExpired (index.js:195)

the errors were populated when I used the middle-clicked the tab header

Is it related to this change ?

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Sep 16, 2019

Is it related to this change ?

I don't think so, all the PR does it prevent the middle click from closing the editor.
I believe it may be related to #4517. It looks like middle clicks in the tab sometimes adds content into the editor from the clipboard.

Do you see content in the editor after?

@lmcbout
Copy link
Contributor

lmcbout commented Sep 17, 2019

Tested on Ubuntu 16.04 and Chrome
I didn't get the error @elaihau had, but if I make a change and don't save it, when I try to close the preferences, I have two popup window asking Your changes will be lost if you don't save them.
I should only have one popup when I select Don't save option

On Firefox:
The middle button insert the clipboard to the preference making the preferences dirty.

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Sep 17, 2019

@lmcbout

I didn't get the error @elaihau had, but if I make a change and don't save it, when I try to close the preferences, I have two popup window asking Your changes will be lost if you don't save them.
I should only have one popup when I select Don't save option

This isn't a behavior introduced through the PR.
The issue is already recorded and written by you #2780

On Firefox:
The middle button insert the clipboard to the preference making the preferences dirty.

Also not an issue with this PR, see #4517.

Copy link
Contributor

@elaihau elaihau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the followings:

  • middle click doesn’t close the tab any more
  • middle click does not paste texts into editors
  • docked the pref editor in different places and I didn’t observe inconsistent behaviours in terms of the two bullet points above

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the two observations are tracked in others PR
Ready to merge.

@elaihau elaihau merged commit d306a29 into master Sep 18, 2019
@elaihau elaihau deleted the vf/GH-2639 branch September 18, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should disable closing preferences editors from middle mouse click
3 participants