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

[sonyprojector] Allow translation of exception messages that can be d… #11392

Merged
merged 2 commits into from
Nov 12, 2021
Merged

[sonyprojector] Allow translation of exception messages that can be d… #11392

merged 2 commits into from
Nov 12, 2021

Conversation

lolodomo
Copy link
Contributor

…isplayed by MainUI

Signed-off-by: Laurent Garnier lg.hc@free.fr

@cweitkamp
Copy link
Contributor

cweitkamp commented Oct 16, 2021 •
edited
Loading

@lolodomo It might make sense to add a solution for translatable exceptions in OHC. wdyt?

@lolodomo lolodomo force-pushed the sonyproj_exception_i18n branch 4 times, most recently from dd9d28d to 90312b9 Compare October 16, 2021 15:07
@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Oct 16, 2021
@lolodomo lolodomo force-pushed the sonyproj_exception_i18n branch from 90312b9 to 1f6c84e Compare October 16, 2021 17:36
@lolodomo lolodomo removed the work in progress A PR that is not yet ready to be merged label Oct 16, 2021
@lolodomo lolodomo force-pushed the sonyproj_exception_i18n branch from 1f6c84e to 04d948c Compare October 17, 2021 07:27
@lolodomo
Copy link
Contributor Author

lolodomo commented Oct 17, 2021 •
edited
Loading

It might make sense to add a solution for translatable exceptions in OHC. wdyt?

@cweitkamp : look at my class I18nException, it could be moved to OHC I think.
By extending this class in any binding (like here with my class SonyProjectorException ), you don't have to implement the getLocalizedMessage but just call the appropriate constructors.
WDYT ?

@lolodomo lolodomo force-pushed the sonyproj_exception_i18n branch 2 times, most recently from 1d33508 to 677b87e Compare October 17, 2021 07:54
@lolodomo
Copy link
Contributor Author

look at my class I18nException, it could be moved to OHC I think.
By extending this class in any binding (like here with my class SonyProjectorException ), you don't have to implement the getLocalizedMessage but just call the appropriate constructors.
WDYT ?

@cweitkamp : any feedback ?

cweitkamp
cweitkamp reviewed Oct 21, 2021
View reviewed changes
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

I looked into I18nException class and its usage. T.b.h I do not understand why we should go for the exception message to be the key of the translatable object. Why don't you use the know @text/exception.whatever pattern?

...ng.sonyprojector/src/main/java/org/openhab/binding/sonyprojector/internal/I18nException.java Outdated Show resolved Hide resolved
...ng.sonyprojector/src/main/java/org/openhab/binding/sonyprojector/internal/I18nException.java Outdated Show resolved Hide resolved
...ng.sonyprojector/src/main/java/org/openhab/binding/sonyprojector/internal/I18nException.java Outdated Show resolved Hide resolved
@lolodomo
Copy link
Contributor Author

I think I will consider a different approach.

@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Oct 21, 2021
@lolodomo lolodomo force-pushed the sonyproj_exception_i18n branch 2 times, most recently from c4c8d20 to 7b7ae33 Compare October 21, 2021 21:30
@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Oct 21, 2021
@lolodomo lolodomo force-pushed the sonyproj_exception_i18n branch from 7b7ae33 to fc0068c Compare October 22, 2021 12:02
@lolodomo lolodomo removed the work in progress A PR that is not yet ready to be merged label Oct 22, 2021
@lolodomo
Copy link
Contributor Author

lolodomo commented Oct 22, 2021 •
edited
Loading

@cweitkamp : new version available that considers your previous comments.

Edit: maybe you have a better name for the method getMessageOrKeyWithParams ?

@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Oct 23, 2021
@lolodomo
Copy link
Contributor Author

New improvement in preparation...

@lolodomo lolodomo force-pushed the sonyproj_exception_i18n branch from fc0068c to 4903c74 Compare October 23, 2021 08:42
@lolodomo lolodomo removed the work in progress A PR that is not yet ready to be merged label Oct 23, 2021
@lolodomo
Copy link
Contributor Author

@cweitkamp : ok, this time, I think it is good ;)

@lolodomo lolodomo force-pushed the sonyproj_exception_i18n branch from 4903c74 to 1fd3784 Compare October 23, 2021 09:24
@lolodomo
Copy link
Contributor Author

@kaikreuzer @wborn : the exit code 137 is back...

@kaikreuzer
Copy link
Member

Yeah, because I allowed 2 executors on agent2 as a try to speed it up. It's already changed back to 1...

@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Oct 23, 2021
@kaikreuzer
Copy link
Member

@lolodomo Build now succeeded, but waiting for @cweitkamp's feedback now.

@kaikreuzer kaikreuzer requested a review from cweitkamp October 25, 2021 06:04
@lolodomo
Copy link
Contributor Author

@cweitkamp : if the current proposal is ok for you, I am going to put this class I18nException in the core repo so that any binding can use it.

@lolodomo
Copy link
Contributor Author

@cweitkamp : for your information, I created openhab/openhab-core#2549

@lolodomo lolodomo added the awaiting other PR Depends on another PR label Oct 31, 2021
@lolodomo
[sonyprojector] Allow translation of exception messages that can be d…
5d57908 
…isplayed in MainUI

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo force-pushed the sonyproj_exception_i18n branch from 0cac2a8 to 5d57908 Compare November 11, 2021 19:09
@lolodomo
Copy link
Contributor Author

Awaiting openhab/openhab-core#2549

@wborn wborn added rebuild Triggers Jenkins PR build and removed awaiting other PR Depends on another PR rebuild Triggers Jenkins PR build labels Nov 12, 2021
wborn
wborn requested changes Nov 12, 2021
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. 👍
Can you address these issues?

...es/org.openhab.binding.sonyprojector/src/main/resources/OH-INF/i18n/sonyprojector.properties Outdated Show resolved Hide resolved
...org.openhab.binding.sonyprojector/src/main/resources/OH-INF/i18n/sonyprojector_fr.properties Outdated Show resolved Hide resolved
.../binding/sonyprojector/internal/communication/serial/SonyProjectorSerialOverIpConnector.java Outdated Show resolved Hide resolved
...rg/openhab/binding/sonyprojector/internal/communication/sdcp/SonyProjectorSdcpConnector.java Outdated Show resolved Hide resolved
...in/java/org/openhab/binding/sonyprojector/internal/communication/SonyProjectorConnector.java Outdated Show resolved Hide resolved
...in/java/org/openhab/binding/sonyprojector/internal/communication/SonyProjectorConnector.java Outdated Show resolved Hide resolved
...in/java/org/openhab/binding/sonyprojector/internal/communication/SonyProjectorConnector.java Outdated Show resolved Hide resolved
@lolodomo
Consider review comments
66df975 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
wborn
wborn approved these changes Nov 12, 2021
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thanks!

@wborn wborn merged commit 99144d6 into openhab:main Nov 12, 2021
@wborn wborn added this to the 3.2 milestone Nov 12, 2021
@lolodomo lolodomo deleted the sonyproj_exception_i18n branch November 13, 2021 00:33
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Dec 30, 2021
@lolodomo @NickWaterton
[sonyprojector] Allow translation of exception messages that can be d… (
2ff7f0a 
openhab#11392)

* [sonyprojector] Allow translation of exception messages that can be displayed in MainUI

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
mischmidt83 pushed a commit to mischmidt83/openhab-addons that referenced this pull request Jan 9, 2022
@lolodomo @mischmidt83
[sonyprojector] Allow translation of exception messages that can be d… (
5441d3f 
openhab#11392)

* [sonyprojector] Allow translation of exception messages that can be displayed in MainUI

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Michael Schmidt <mi.schmidt.83@gmail.com>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jan 28, 2022
@lolodomo
[sonyprojector] Allow translation of exception messages that can be d… (
c1238be 
openhab#11392)

* [sonyprojector] Allow translation of exception messages that can be displayed in MainUI

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
@lolodomo
[sonyprojector] Allow translation of exception messages that can be d… (
ebf1848 
openhab#11392)

* [sonyprojector] Allow translation of exception messages that can be displayed in MainUI

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
@lolodomo @andan67
[sonyprojector] Allow translation of exception messages that can be d… (
118543c 
openhab#11392)

* [sonyprojector] Allow translation of exception messages that can be displayed in MainUI

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cweitkamp cweitkamp cweitkamp left review comments

@wborn wborn wborn approved these changes

Assignees
No one assigned
Labels
translation
Projects
None yet
Milestone
3.2
Development

Successfully merging this pull request may close these issues.
4 participants
@lolodomo @cweitkamp @kaikreuzer @wborn