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

Support for Qute Type-safe Message Bundles #845

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

angelozerr
Copy link
Contributor

Support for Qute Type-safe Message Bundles

Fixes #800

@angelozerr
Copy link
Contributor Author

The support for Qute Type-safe Message Bundles starts working with Java interfaces (but not with properties file).

See the following demo:

MessageWithJavaInterfaceDemo

@mkouba @FroMage I have started the support for Qute Type-safe Message Bundles which works only with Java interface. I need to support properties file.

Any feedback are welcome if you need some feature.

@angelozerr
Copy link
Contributor Author

angelozerr commented Mar 31, 2023

@mkouba I have tried to play with @MessageBundle and I have several questions:

  • could we define 2 interfaces wich implements @MessageBundle and the other @MessageBundle("msg1"). I tried it and to write expression like {msg:hello} and {msg1:hello} but it doesnt work.
  • according my test, you need to write an interface which implement @MessageBundle even if you wish to use msg:message (with a key). Is it normal? I have this error:

No namespace resolver found for [msg] in expression {msg:message('hello_name',name)}

@angelozerr
Copy link
Contributor Author

angelozerr commented Apr 2, 2023

I have improved the support and now you can see message content as inlay hint in the template:

image

@mkouba @FroMage @fbricon @datho7561 @JessicaJHee for now I display as inlay hint the message but I wonder if it should be better to display the evaluaded message .

For instance:

{msg:hello_name('Lucie')} [Hello Lucie!]

If yes what about when name comes from a variable like

{msg:hello_name(person.name)}

Should we display:

  • {msg:hello_name(person.name)} [Hello person.name!]
  • or don't evaluate to display {msg:hello_name(person.name)} [Hello {name}!]

@mkouba
Copy link
Collaborator

mkouba commented Apr 3, 2023

I have improved the support and now you can see message content as inlay hint in the template:

image

@mkouba @FroMage @fbricon @datho7561 @JessicaJHee for now I display as inlay hint the message but I wonder if it should be better to display the evaluaded message .

For instance:

{msg:hello_name('Lucie')} [Hello Lucie!]

If yes what about when name comes from a variable like

{msg:hello_name(person.name)}

Should we display:

* `{msg:hello_name(person.name)}  [Hello person.name!]`

* or don't evaluate to display `{msg:hello_name(person.name)}  [Hello {name}!]`

I think that we should display the message template and not the evaluaded message. Mainly because we are not always able to evaluate the message template and then it might become confusing... (the {msg:hello_name(person.name)} [Hello person.name!] example).

@angelozerr
Copy link
Contributor Author

I think that we should display the message template and not the evaluaded message. Mainly because we are not always able to evaluate the message template and then it might become confusing... (the {msg:hello_name(person.name)} [Hello person.name!] example).

Ok thanks for your feedback @mkouba !

@angelozerr
Copy link
Contributor Author

angelozerr commented Apr 3, 2023

I need to clean my code and write tests. This PR provides the first support for Qute messages but it need to be improved with:

  • support for locale
  • support as inlay hint message when it is defined in a msg.properties file.
  • code action to create an AppMessages when it doesn't exists.
  • support msg:message completion
  • other?

I will need that in separate PRs to avoid having a big PR.

@datho7561 @JessicaJHee you can start to play with this PR. Once tests and code will be cleaned I will notice you.

@angelozerr
Copy link
Contributor Author

To play with Qute messages, I suggest that you use the qute-messages that I will use for Qute JDT tests.

@angelozerr
Copy link
Contributor Author

@FroMage
Copy link

FroMage commented Apr 3, 2023

I think that we should display the message template and not the evaluaded message. Mainly because we are not always able to evaluate the message template and then it might become confusing... (the {msg:hello_name(person.name)} [Hello person.name!] example).

I don't know. I was going to answer the other way, and display the text substituted. At a point (in the view) where we don't know what {name} maps to, because we don't know the name of the parameters, I find it more useful to display Hello Lucie for constants, and Hello {person.name} for non-constant expressions.

@angelozerr angelozerr force-pushed the messages branch 2 times, most recently from 573c5c6 to af9bddc Compare April 3, 2023 14:32
@angelozerr
Copy link
Contributor Author

I don't know. I was going to answer the other way, and display the text substituted. At a point (in the view) where we don't know what {name} maps to, because we don't know the name of the parameters, I find it more useful to display Hello Lucie for constants, and Hello {person.name} for non-constant expressions.

It was my original idea by @mkouba think we should not evaluate the message #845 (comment)

@angelozerr angelozerr force-pushed the messages branch 3 times, most recently from 0f9304b to b3cf09d Compare April 3, 2023 18:06
@angelozerr angelozerr marked this pull request as ready for review April 3, 2023 18:07
@angelozerr
Copy link
Contributor Author

@datho7561 @JessicaJHee the PR can be reviewed now, code is clean and tests exists now.

Copy link
Contributor

@JessicaJHee JessicaJHee left a comment

Choose a reason for hiding this comment

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

I played around and looks great! I just noticed some very minor things.

I think this feature you mentioned would be very nice to have in a future PR:

  • support as inlay hint message when it is defined in a msg.properties file.

@angelozerr
Copy link
Contributor Author

I played around and looks great!

Thanks!

I think this feature you mentioned would be very nice to have in a future PR:

It is in my plan, but this PR is already big, so I prefer providing message support in separate PRs to have easier review.

@angelozerr
Copy link
Contributor Author

@JessicaJHee I have improved inlay hint and now you click on it to edit the Java message by going to the java method.

MessageInlayHintClickable

It requires to improve it to set the location to the @message annotation content and not the Java method but we can do that in a separate PR (if you are interested you could work on it).

Once message properties support will be available #847 my idea is to provide the capability to open the msg.properties file.

Fixes redhat-developer#800

Signed-off-by: azerr <azerr@redhat.com>
@angelozerr
Copy link
Contributor Author

@datho7561 @JessicaJHee can we merge this PR?

@datho7561
Copy link
Contributor

I haven't had a chance to look at it yet, but I know that Jessica reviewed it. Do you think it's ready to merge, Jessica?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initialize support for Qute Type-safe Message Bundles
5 participants