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

provide a way to ask client to show modal message box #1337

Open
heejaechang opened this issue Aug 26, 2021 · 10 comments
Open

provide a way to ask client to show modal message box #1337

heejaechang opened this issue Aug 26, 2021 · 10 comments
Labels
feature-request Request for new features or functionality show message
Milestone

Comments

@heejaechang
Copy link

currently, ShowXXXMessage API from RemoteWindow doesn't let server ask client to show modal message box and it is completely up to client.

It would be nice if one can ask client to show modal window instead of leaving it to client.

the usage case for us is we need to decide whether we should proceed with some expansive refactoring after user action, so we want to ask the user but prevent the user from changing code before answer.

@rwols
Copy link
Contributor

rwols commented Aug 26, 2021

Is this a duplicate of #1164?

@heejaechang
Copy link
Author

@rwols that would be superset of what I am asking. since the issue is asking much more than what I am asking. probably requiring a lot more discussion. ShowXXXMessage already exist and having MessageOption like what vscode offer seems much smaller, incremental change that doesn't have big impact.

@dbaeumer
Copy link
Member

I personally think it is a bad idea to let servers open modal dialogs. The reason is that modal dialogs should only be shown in close relation to a user UI interaction. Modal dialogs that pop up without that close relation ship cause lots of confusion (I once did this in ESLint and got very bad feedback for it :-)).

So I would recommend that you in the extension code intercept the rename and might do some custom handling and show the modal dialog there. Would that work for you?

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Aug 27, 2021
@heejaechang
Copy link
Author

@dbaeumer ya, custom LSP and command is what I am doing for now, but that basically requires adding this special handling for all clients that we want to support this feature. that was the part I would like to remove in future. hence the asking.

@dbaeumer
Copy link
Member

I see your point but allowing servers to open a modal dialogs is really something I would like to avoid.

@dbaeumer dbaeumer added feature-request Request for new features or functionality show message and removed info-needed Issue requires more information from poster labels Aug 31, 2021
@dbaeumer dbaeumer added this to the Backlog milestone Aug 31, 2021
@DanTup
Copy link
Contributor

DanTup commented Sep 8, 2021

I don't know if it'd work for you, but Dart uses the document version number to confirm the file was not modified while waiting for a response from refactor confirmations:

https://github.com/dart-lang/sdk/blob/3aa464af26a4467d3977150108c1a9c7b0b1a53a/pkg/analysis_server/lib/src/lsp/handlers/handler_rename.dart#L171-L204

I think this would work better than a modal dialog, as the user may have already modified the file in between the refactor request being sent to the server, and the servers request for a modal being processed by the client (and while the server can delay processing that edit, the refactor would still compute edits based on contents that are different to the editor has, so they'd either be rejected anyway, or potentially corrupt the users code).

@heejaechang
Copy link
Author

our rename touches more than one file in the workspace (open or closed) so simple client version check doesn't work, but feels like checking might be better than modal window if there is a way.

@DanTup
Copy link
Contributor

DanTup commented Sep 8, 2021

If you really want to invalidate the rename on any file edit, perhaps you could keep a single global "workspace version" that you just increase on each textDocument/change (and perhaps open if the content may vary from what's on disk) and then grab that before/after the prompt to see if anything was changed.

Of course, neither that or a modal prompt would prevent files being modified on-disk outside of the editor during that period.

@michaelmesser
Copy link
Contributor

I personally think it is a bad idea to let servers open modal dialogs. The reason is that modal dialogs should only be shown in close relation to a user UI interaction. Modal dialogs that pop up without that close relation ship cause lots of confusion (I once did this in ESLint and got very bad feedback for it :-)).

Just because modal dialogs are the wrong solution to some problems, doesn't need they aren't the right solution to other problems.

So I would recommend that you in the extension code intercept the rename and might do some custom handling and show the modal dialog there. Would that work for you?

Isn't the point of LSP to not need custom code in extensions?

@dbaeumer
Copy link
Member

Isn't the point of LSP to not need custom code in extensions?

LSP is about standardizing programming language messages and not UI. I would like to leave the UI part to the client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality show message
Projects
None yet
Development

No branches or pull requests

5 participants