Vojtech Szocs has posted comments on this change.
Change subject: webadmin: user portal and webadmin localization
......................................................................
Patch Set 7:
Hi Alona, great work, this beats my 1.4 MB patch size record :-P (you now hold
the record)
I went over the changes. I have some questions for you:
* Why was UICommonWeb.gwt.xml deleted? After this, GWT compile will fail with
GWT projects depending on UiCommonWeb (WebAdmin & UserPortal-GWTP). Or am I
missing something here?
* In Constants/Messages interfaces for the Frontend (gwt-common,
userportal-gwtp, webadmin), we're already using @DefaultStringValue for all
message keys. Do we really need to have default Constant/Messages properties
files as well with exact same values? If we keep using @DefaultStringValue, we
can just have locale-specific properties, e.g.
"ApplicationConstants_es.properties" etc. Maintaining both @DefaultStringValue
and default properties will not be easy..
* Besides using Constants/Messages interfaces throughout Frontend and UiCommon,
are there any other major infrastructure changes? I tried to look but didn't
find any (I might have missed something though).
* In Enums.properties, there is some ">>>>>>> webadmin" stuff, I guess because
of some rebase?
* In ConstantsManager, I'd add a private constructor and make ConstantsManager
final to indicate it shouldn't be used directly, but via getInstance() (just a
suggestion).
Here are my other notes, just for my understanding of the patch and its
implications:
* Currently, we have 2 ways to localize View/UiBinder component: 1) have
*.ui.xml declare <ui:with...> for constants, or 2) have localize() method in
View. I guess they are pretty much equal, we originally followed approach 2)
but I saw you also used 1). On second thought 1) seems easier, despite the fact
that <ui:with...> GWT.create()'s constants for each component, in the end,
AFAIK constants are implemented via singleton.
* I guess NLS comment stuff ("//$NON-NLS-N$") doesn't apply to strings in
annotations, e.g. @DefaultStringValue("..."), is this right?
Thanks :) Vojtech
--
To view, visit http://gerrit.ovirt.org/3612
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I74c56a3aed35dec0a378efc58a2e44569eff6089
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
