Allow suppressing UI blocking dialogs #2546

eugenesvk · 2024-11-06T09:58:19Z

Add 2 user configuration options that would allow to suppress the attention-demanding UI blocking dialogs, instead redirecting warnings/errors to a status message and the full console message

Simple way to close #2545 while preserving the current defaults

Though not all modals are affected since some are invoked by the lsp_utils dependency

Question: is it possible to make that dependency respect config options from this plugin? Or do dependencies "by design" ignore their clients?

…ssage

rchl · 2024-11-06T13:48:03Z

plugin/tooling.py

-            sublime.error_message("The clipboard content must be a URL to a package.json file.")
+            msg = "The clipboard content must be a URL to a package.json file."
+            status = "Clipboard must be a URL to package.json"
+            notify_err(msg, status)


I honestly don't think we are gonna accept any of these changes (not in this form). It doesn't make sense to just blindly change all instance of modals. Especially in this file it's a case of explicit user action and it makes sense to report the error in the dialog (the user won't be surprised by it) instead of having to dig for more info in the console or potentially miss the status field message.

The users to whom it makes sense to see these kind of dialogs or who worry about missing a status field message can simply continue to use the defaults. Or they can set the regular dialog suppression setting, but leave the error setting on.

So this setting is simply not for them, but for the other group who have never had an issue of missing a status field message (by the way, you could add ❗ symbol if you think this is a very important message, that's harder to miss), don't see the point of a blocking modal for a simple error message. For them these bad interface design patterns don't make sense because they do not reflect the importance of the messages.

explicit user action and it makes sense to report the error in the dialog (the user won't be surprised by it)

The issue here isn't surprise, but, requiring additional user action for no good reason.

What's your alternative - group/code all the messages and ask the user to provide a list of those that should be suppressed like in lints?

In this case the error is a direct response to user action so it's fine to present a dialog that can be dismissed easily with a single button press.

If you don't like dialogs so much then maybe just create a plugin that overrides sublime.error_message (if that's possible but I think it should)? It's not sensible to expect every package to introduce an option to switch between error dialogs and status field messages.

For some cases I would also prefer to not have dialogs but as discussed in related conversation, there is not necessarily a better way due to ST's limitations.

In this case the error is a direct response to user action

Showing a status message is also a direct response

so it's fine to present a dialog that can be dismissed easily with a single button press.

It's not fine, you need to provide value to the user to demand action from every single user (this is an optional config, disabled by default, so most of the users will continue to suffer through needless actions the way you want them), and in this case you don't - this is simply not an important error message.

Also for some of these cases a blocking message prevents a much better UI: for example, in the next error that appears after theparse VSCode package JSON action instead of a blocking URL must end with 'package.json'" it'd be better to show the same input field again with the old user value and show a non-blocking error message (e.g., in the status bar or maybe in some popup) so you don't lose valuable user input data and allow for the next user action to be error correction instead of UI unblocking

If you don't like dialogs so much then maybe just create a plugin that overrides sublime.error_message (if that's possible but I think it should)?

I don't like needless user actionn, not dialogs, so I don't want to override all the error messages, just the ones that aren't important. None in your plugin are, so I'd be fine with overriding all of them.

It's not sensible to expect every package to introduce an option to switch between error dialogs and status field messages.

I don't expect that, there are plenty of plugins that don't show dialogs, or use less intrusive notifications, and this is a PR for a single package optionally supressing unimportant error messages, not for Sublime core changing everything

jwortmann · 2024-11-06T16:40:52Z

Here is my opinion:

I would probably be okay with such a change to remove (most of) the dialog windows, which don't require user input.
if we want to make this configurable, it should be a single setting and use a better name. Maybe something like suppress_error_dialogs. No need to distinguish between message_dialog and error_dialog imo. But I would probably favor not to add a setting at all for this, unless you want to implement a "don't show this dialog again" checkbox/button.
the status text should give a reference to the console, like "Failed to start LSP-json - see console for details"
status message should use the Window.status_message method if possible, not sublime.status_message. And there are likely a few more things to clean up in the code, e.g. don't use local imports. But I haven't looked too closely yet.
I don't really like emojis as ⚠️ in the status bar or console
I found the package for notifications: https://facelessuser.github.io/SubNotify/ but unfortunately it is a separate package and not a dependency. While it could be implemented with a fallback (https://facelessuser.github.io/SubNotify/usage/#tips-and-tricks-for-developers), probably no user has that package installed, so it seems more or less useless to use as it is now. Probably out of scope for this PR, but maybe it could be converted to a regular dependency?

jwortmann · 2024-11-06T16:57:02Z

Question: is it possible to make that dependency respect config options from this plugin? Or do dependencies "by design" ignore their clients?

Yes, should be possible. Just load settings via sublime.load_settings('LSP.sublime-settings').

show became suppress, so dialogs reversed with status messages

LSP.sublime-settings

plugin/code_actions.py

eugenesvk · 2024-11-07T08:08:28Z

Updated the PR per the following:

I would probably be okay with such a change to remove (most of) the dialog windows, which don't require user input.

switched to suppress by default

if we want to make this configurable, it should be a single setting and use a better name. Maybe something like suppress_error_dialogs. No need to distinguish between message_dialog and error_dialog imo. But I would probably favor not to add a setting at all for this, unless you want to implement a "don't show this dialog again" checkbox/button.

Left the setting given that it's suppressed by default, also re the extra button: Sublime still hasn't learned to properly roundtrip user configs, and this button would change files in the User folder, thus breaking subtle formatting, so I'd just leave it in the currently less convenient format

the status text should give a reference to the console, like "Failed to start LSP-json - see console for details"

Added a shorter text, otherwise it might not fit

status message should use the Window.status_message method if possible, not sublime.status_message. And there are likely a few more things to clean up in the code, e.g. don't use local imports. But I haven't looked too closely yet.

switched

I don't really like emojis as ⚠️ in the status bar or console

They address @rchl concern of users not noticing notifications, ⚠️ make them stand out. I don't mind either way, so removed ⚠️, but left ❗ as I presume those are supposed to still be differentiated somehow

rchl · 2024-11-07T08:19:18Z

I don't really like emojis as ⚠️ in the status bar or console

They address @rchl concert of users not noticing notifications, ⚠️ make them stand out. I don't mind either way, so removed ⚠️, but left ❗ as I presume those are supposed to still be differentiated somehow

I do think that there will be a good portion of users that will not notice something showing up in the status field for a few seconds. Or if they do notice something changing, they might not realize what has changed (if the status field is busy).

So as I said, I don't think that status messages are a good solution in general.

I suppose when user is doing an explicit action then he/she might be more looking for some feedback/result and might pay more attention.

If the user doesn't expect anything then it might be easy to miss status change completely.

eugenesvk · 2024-11-07T08:52:30Z

After checking the manually shortened messages, there is actually no ❗ left, only in the short template when the only thing you see is ❗LSP: see console log… instead of any substatntive message, and since you both don't like them won't adde them to the actual status error messages

I do think that there will be a good portion of users that will not notice something showing up in the status field for a few seconds

Have you actually tried it? It's harder to miss colored ❗ motion in your vision periphery

So as I said, I don't think that status messages are a good solution in general.

By the way, for very important messages you could also use these popups, that's in user's focus (at the cursor) (and of course it could fit the whole message)

fallback sublime functions

jwortmann · 2024-11-07T14:47:32Z

plugin/core/logging.py

+    from .settings import userprefs
+    if userprefs().suppress_error_dialogs:
+        if window:
+            window.status_message(status)  # print short message to statusbar


I don't think we need those inline comments because it's obvious from the method name.

jwortmann · 2024-11-07T14:54:57Z

plugin/core/logging.py

@@ -39,3 +39,33 @@ def exception_log(message: str, ex: Exception) -> None:
 def printf(*args: Any, prefix: str = 'LSP') -> None:
    """Print args to the console, prefixed by the plugin name."""
    print(prefix + ":", *args)
+
+
+def notify(window: Optional[sublime.Window], message: str, status: str = 'LSP: see console log…') -> None:


Should be sublime.Window | None

I'd say status doesn't need a default value if it given in each function call anyway. Maybe rename to status_message

jwortmann · 2024-11-07T14:57:00Z

plugin/core/logging.py

+    """Pick either of the 2 ways to show a user notification message:
+      - via a detailed console message and a short status message
+      - via a blocking modal dialog"""
+    from .settings import userprefs


Can this be a global import at the top of the file? If there is an import cycle it might be necessary to refactor / move some code around between the files.

jwortmann · 2024-11-07T15:01:58Z

plugin/core/logging.py

+        sublime.message_dialog(message)
+
+
+def notify_error(window: Optional[sublime.Window], message: str, status: str = '❗LSP: see console log…') -> None:


I think we don't need 2 separate functions. Current use of sublime.mesage_dialog and sublime.error_message looks pretty arbitrary. I think all of them are errors, so I'd use only sublime.error_message. If the severity is important it could be another parameter severety, but I don't think that's necessary. notify_error sounds like the appropriate function name, so I would remove the other function.

jwortmann · 2024-11-07T15:07:15Z

plugin/core/logging.py

+        else:
+            sublime.status_message(status)


I assume you used this to satisfy the type checker. I believe window = None is only a theoretical case, and if there is no (active) window, I think we don't need to log anyway. So you can just put

if not window: return

at the top of the function.

jwortmann · 2024-11-07T15:11:01Z

plugin/core/windows.py

@@ -295,12 +296,13 @@ def start_async(self, config: ClientConfig, initiating_view: sublime.View) -> No
                "Re-enable by running \"LSP: Enable Language Server In Project\" from the Command Palette.",
                "\n\n--- Error: ---\n{1}"
            )).format(config.name, str(e))
+            status = f"LSP: Failed to start {config.name}… See console"


Do we need the LSP: prefix in the status messages? It should be obvious if there is either the config.name or when it was something directly triggered by the user.

eugenesvk added 4 commits November 6, 2024 15:49

add 'show_ui_blocking_message' config

52e9f01

allow replacing blocking dialog modals with a status+console message

c9547c6

add 'show_ui_blocking_err_message' config

1911145

allow replacing blocking error dialog modals with a status+console me…

165fa1f

…ssage

eugenesvk mentioned this pull request Nov 6, 2024

Disable blocking popup modal errors by default #2545

Open

rchl reviewed Nov 6, 2024

View reviewed changes

eugenesvk added 7 commits November 7, 2024 15:01

add "see console"

bddc664

use window instead of sublime for message dialogs

81c05d0

use window instead of sublime for error dialogs

848736b

remove ⚠️ for messages (leave ❗ for errors)

e3b7fc0

combine two blocking options into one suppress_error_dialogs

1847753

suppress dialogs by default

cc62214

swap logging logick to reflect the settings change

cccc173

show became suppress, so dialogs reversed with status messages

rchl reviewed Nov 7, 2024

View reviewed changes

LSP.sublime-settings Outdated Show resolved Hide resolved

rchl reviewed Nov 7, 2024

View reviewed changes

plugin/code_actions.py Outdated Show resolved Hide resolved

move setting to other

bd23db9

use longer names

5876ba0

eugenesvk force-pushed the fr-no-modal branch from 31a5fd2 to 5876ba0 Compare November 7, 2024 10:57

eugenesvk added 3 commits November 7, 2024 18:56

add missing window arg

360c41a

accept optional sublime Window in notify functions

d9dc495

fallback sublime functions

fix style

88f4c62

eugenesvk force-pushed the fr-no-modal branch from 86acd06 to 88f4c62 Compare November 7, 2024 12:19

jwortmann reviewed Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow suppressing UI blocking dialogs #2546

Allow suppressing UI blocking dialogs #2546

eugenesvk commented Nov 6, 2024 •

edited

Loading

rchl Nov 6, 2024 •

edited

Loading

eugenesvk Nov 6, 2024

rchl Nov 6, 2024

eugenesvk Nov 6, 2024

jwortmann commented Nov 6, 2024

jwortmann commented Nov 6, 2024

eugenesvk commented Nov 7, 2024 •

edited

Loading

rchl commented Nov 7, 2024

eugenesvk commented Nov 7, 2024

jwortmann Nov 7, 2024

jwortmann Nov 7, 2024

jwortmann Nov 7, 2024

jwortmann Nov 7, 2024

jwortmann Nov 7, 2024

jwortmann Nov 7, 2024

		sublime.message_dialog(message)


		def notify_error(window: Optional[sublime.Window], message: str, status: str = '❗LSP: see console log…') -> None:

Allow suppressing UI blocking dialogs #2546

Are you sure you want to change the base?

Allow suppressing UI blocking dialogs #2546

Conversation

eugenesvk commented Nov 6, 2024 • edited Loading

rchl Nov 6, 2024 • edited Loading

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

jwortmann commented Nov 6, 2024

jwortmann commented Nov 6, 2024

eugenesvk commented Nov 7, 2024 • edited Loading

rchl commented Nov 7, 2024

eugenesvk commented Nov 7, 2024

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

eugenesvk commented Nov 6, 2024 •

edited

Loading

rchl Nov 6, 2024 •

edited

Loading

eugenesvk commented Nov 7, 2024 •

edited

Loading