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

Cleanup fmxlview and javax imports #3661

Merged

Conversation

bodymindarts
Copy link

Removes redundant @FxmlView from abstract view classes and consitently imports DI annotations from javax package.

lusarz
lusarz previously approved these changes Nov 22, 2019
Copy link
Contributor

@lusarz lusarz left a comment

Choose a reason for hiding this comment

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

For me looks great :)

@stejbac
Copy link
Contributor

stejbac commented Nov 22, 2019

The @Named DI annotations on the constructor params of AgentRegistrationView and DisputeAgentView are presumably also not needed. (Also can remove redundant FxmlView imports.)

Copy link
Member

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

utACK assuming a valid answer to my comment.

@@ -54,7 +50,7 @@ public DisputeAgentView(DisputeManager<? extends DisputeList<? extends DisputeLi
ContractWindow contractWindow,
TradeDetailsWindow tradeDetailsWindow,
AccountAgeWitnessService accountAgeWitnessService,
@Named(AppOptionKeys.USE_DEV_PRIVILEGE_KEYS) boolean useDevPrivilegeKeys) {
Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious to me why injection of this value would still work without the explicit annotation. The value of AppOptionKeys.USE_DEV_PRIVILEGE_KEYS is in fact "useDevPrivilegeKeys", spelled exactly the same as the useDevPrivilegeKeys parameter, so in theory Guice could introspect and do the injection based on that, but this information is not inspectable at runtime unless the -parameters option has been passed to javac to keep the parameter symbol table available (and our build does not do this). What am I missing?

Copy link
Author

Choose a reason for hiding this comment

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

The class is abstract and never instantiated directly. The annotation is legacy from a time before the type hierarchy was in place.

Copy link
Author

Choose a reason for hiding this comment

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

The sub-classes should have the annotation.

Copy link
Member

Choose a reason for hiding this comment

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

Roger that, and I see that the subclasses are in fact annotated. Thanks.

utACK

Copy link
Member

@freimair freimair left a comment

Choose a reason for hiding this comment

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

Ack

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.

None yet

6 participants