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.

Sign up for GitHub

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

Jump to bottom

[vscode] support AuthenticationForceNewSessionOptions #12752

Merged
merged 1 commit into from
Jul 27, 2023
Merged

[vscode] support AuthenticationForceNewSessionOptions #12752

merged 1 commit into from
Jul 27, 2023

Conversation

rschnekenbu
Copy link
Contributor

@rschnekenbu rschnekenbu commented Jul 25, 2023 •
edited by vince-fugnitto
Loading

What it does

Support of AuthenticationForceNewSessionOptions
A detailed message may be now given rather than a simple boolan when forcing a new session.

Fixes #12612

Contributed on behalf of ST Microelectronics

How to test

  1. Install following extension:

  2. The extension provides a set of commands that simulate the login using a dummy authentification provider. These commands all have a label starting with 'Auth-Test'.

    • Get all sessions lists all current sessions or undefined if none exist
    • Get session with no option gets the session with the default options. It won't create a new one by default, so no session will be displayed if used first
    • Get session with creationIfNone will get any existing session or create a new one if none exists
    • Get session with forceNewSession will force a new session (forceNewSession is true). Prompt a message with default content.
    • Get session with forceNewSession and detail will force a new session (forceNewSession is AuthenticationForceNewSessionOptions with an existing detail string). Prompt a message with content and detail message.

  3. On master theia, after creating an new session. Invoke Get session with forceNewSession command or Get session with forceNewSession and detail command will always lead to the same result. The message dialog to allow the new session always have the same message.

  4. Switch to this PR and run the same tests. The dialog will contain additional detail in the prompt dialog: 'Message based on detail for the force New Session' if Get session with forceNewSession and detail command is invoked.

Review checklist

Reminder for reviewers

@rschnekenbu rschnekenbu marked this pull request as ready for review July 25, 2023 07:03
@rschnekenbu rschnekenbu mentioned this pull request Jul 25, 2023
Closed
2 tasks
tsmaeder
tsmaeder reviewed Jul 25, 2023
View reviewed changes
packages/plugin-ext/src/main/browser/authentication-main.ts Show resolved Hide resolved
packages/plugin-ext/src/main/browser/authentication-main.ts Outdated
? nls.localizeByDefault("The extension '{0}' wants you to sign in again using {1}.", extensionName, providerName)
: nls.localizeByDefault("The extension '{0}' wants to sign in using {1}.", extensionName, providerName);

message = detail ? message + ' ' + detail : message;
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of questions here:

  1. Shouldn't we include the detail as a parameter of the internationalized message?
  2. What kind of text is typically here: a whole paragraph, a single sentence?
  3. If this text can be longer, shouldn't it be separated from the main text at least with a line break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point for the internationalization! The detail comes from the extension, so should we take care of it or should the detail message already be translated by the extension provider? Not too sure what is the usage here.
For the kind of text, I would expect a single sentence here, not a long paragraph. That's why I simply added after the basic message. That kept the message dialog compact. I think however vscode adds a line separator between the message and the dialog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Internationalization should be performed by extension.
New line is under evaluation. Not trivial to add one currently. (tested '\n' and
without succes)

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked in VS Code: they use a modal dialog and not an info dialog. Using a proper dialog would also allow to format as we like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used a modal dialog rather than a notification. I still have a small issue on the "Allow" button internationalization of this dialog.
In the current version, I do not run any internationalization on "Allow" button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While developing this PR, I stumbled onto an issue with localization. vscode uses the following code for the "Allow" button:
primaryButton: nls.localize({ key: 'allow', comment: ['&& denotes a mnemonic'] }, "&&Allow")

I have the same kind of code on the dialog, using nls.localizeByDefault("&&Allow"). As I do not have any locale on my debug environement, I get "&&Allow" as the text of the button.
@msujew, would you have an idea how I could get the correct text?
https://github.com/eclipse-theia/theia/blob/master/packages/core/src/common/nls.ts#L35 => this part of the code is skipped, as localization is undefined. So we don't normalize the string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tsmaeder, in the current state, I think the internationalization of the button should not remain an issue, as it can be fixed later on. This would let us work on the final steps before the release, asupdating DEFAULT_SUPPORTED_API_VERSION and the other linked elements. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Theia strips any mnemonics off of vscode's original text. You should be able to just use nls.localizeByDefault('Allow') (assuming it's in the nls.metadata.json file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Mark! Removing the mnemonics worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

login-english
Login in English
login-french
Login in French

@vince-fugnitto vince-fugnitto added vscode issues related to VSCode compatibility authentication issues related to authentication (ex: authentication api) labels Jul 25, 2023
@tsmaeder
Copy link
Contributor

@rschnekenbu we have conflicts again.

@rschnekenbu
Copy link
Contributor Author

I will fix the lint issues.

@rschnekenbu rschnekenbu force-pushed the issues/12612 branch 2 times, most recently from 30a847f to 60921c4 Compare July 26, 2023 12:44
@rschnekenbu rschnekenbu requested a review from tsmaeder July 26, 2023 13:13
@tsmaeder
Copy link
Contributor

@rschnekenbu the dialog has no title. That's just odd.

@rschnekenbu
Copy link
Contributor Author

@tsmaeder, indeed! That is the same as vscode, but vscode has a 'default' title to the dialog.

vscode-sample

@rschnekenbu
Copy link
Contributor Author

PR with a new title:
login-french-titled
login-english-titled

@rschnekenbu
[vscode] support AuthenticationForceNewSessionOptions with detail mes…
716274e 
…sage

* Defines and supports this new type in the authentification service.
* Updates prompt when new session is forced if detail message is present.

Contributed on behalf of STMicroelectronics
@rschnekenbu
Copy link
Contributor Author

Thanks for your reviews, @tsmaeder!
I will rebase and squash all commits, so this can be pushed onto latest master.

@tsmaeder tsmaeder merged commit e3ecd7b into eclipse-theia:master Jul 27, 2023
@vince-fugnitto vince-fugnitto added this to the 1.40.0 milestone Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msujew msujew msujew left review comments

@tsmaeder tsmaeder tsmaeder approved these changes

Assignees
No one assigned
Labels
authentication issues related to authentication (ex: authentication api) vscode issues related to VSCode compatibility
Projects
PR Backlog
Archived in project
Milestone
1.40.0
Development

Successfully merging this pull request may close these issues.

[vscode] Support AuthenticationForceNewSessionOptions
4 participants
@rschnekenbu @tsmaeder @msujew @vince-fugnitto