Add missing @Named annotations for CoinFormatter injection #3658

bodymindarts · 2019-11-21T22:00:05Z

I missed a few places where @Named(BTC_FORMATTER_KEY) annotation should be added in my recent PR #3634.

lusarz · 2019-11-22T08:07:08Z

desktop/src/main/java/bisq/desktop/main/support/dispute/DisputeView.java

@@ -139,7 +142,7 @@
    public DisputeView(DisputeManager<? extends DisputeList<? extends DisputeList>> disputeManager,
                       KeyRing keyRing,
                       TradeManager tradeManager,
-                       CoinFormatter formatter,
+                       @Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormatter formatter,


I think in this case it's unnecessary, because this constructor is not annotated with @Inject

That is what I thought too, which is why I left these out originally. But it turns out @inject is not infact mandatory and the program crashes on these views without this annotation. Thanks to @stejbac for discovering this.

Are you sure that app crash when you ommit @Named annotation on this particular file (DisputeView) ? It's an abstract class and it does not "participate" in dependency injection.

On second thought, I might have been wrong about @Inject not being mandatory for Guice, since looking closer the modified Dispute* classes all have subclasses with @Inject-annotated constructors - probably only TransactionsView and TakeOfferView actually needed fixing. (I'm pretty sure it isn't mandatory by default for Spring, but perhaps Guice is stricter.)

Also, there are (probably leftover) @FxmlView annotations on the 3 Dispute* classes which mislead me to think that they were concrete components - perhaps those annotations can be removed?

I think here it is not necessary as mentioned by @lusarz. At least it didn't crash for me here.

But it turns out @inject is not infact mandatory and the program crashes on these views without this annotation.

Did it crash for you while testing?

I agree that probably only these two views needs to be annotated.

Yeah, I suppose @FxmlView on abstract views are leftovers, and it would be nice to remove them.

Inject annotation is needed for the concrete classes but not for the base classes (should not be used there). It probably crashed due some nullpointers caused if the annotation is missing but with view classed you don't get proper error logs.

ripcurlx

NACK - please see my comments

ripcurlx · 2019-11-22T09:38:46Z

desktop/src/main/java/bisq/desktop/main/support/dispute/DisputeView.java

@@ -139,7 +142,7 @@
    public DisputeView(DisputeManager<? extends DisputeList<? extends DisputeList>> disputeManager,
                       KeyRing keyRing,
                       TradeManager tradeManager,
-                       CoinFormatter formatter,
+                       @Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormatter formatter,


I think here it is not necessary as mentioned by @lusarz. At least it didn't crash for me here.

ripcurlx · 2019-11-22T09:39:28Z

desktop/src/main/java/bisq/desktop/main/support/dispute/agent/DisputeAgentView.java

@@ -48,7 +49,7 @@
    public DisputeAgentView(DisputeManager<? extends DisputeList<? extends DisputeList>> disputeManager,
                            KeyRing keyRing,
                            TradeManager tradeManager,
-                            CoinFormatter formatter,
+                            @Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormatter formatter,


I think here it is not necessary.

ripcurlx · 2019-11-22T09:39:49Z

desktop/src/main/java/bisq/desktop/main/support/dispute/client/DisputeClientView.java

 @FxmlView
 public abstract class DisputeClientView extends DisputeView {
    public DisputeClientView(DisputeManager<? extends DisputeList<? extends DisputeList>> DisputeManager,
                             KeyRing keyRing,
                             TradeManager tradeManager,
-                             CoinFormatter formatter,
+                             @Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormatter formatter,


I think here it is not necessary.

ripcurlx · 2019-11-22T09:45:30Z

One more question: Is there a reason why we use for these injects the Named annotation from javax.inject.Named and on other places com.google.inject.name.Named?

ripcurlx · 2019-11-22T09:47:23Z

One more question: Is there a reason why we use for these injects the Named annotation from javax.inject.Named and on other places com.google.inject.name.Named?

It seems both work the same as described in https://github.com/google/guice/wiki/JSR330

lusarz · 2019-11-22T09:52:29Z

@ripcurlx I think there is no reason - I suppose everyone use autoimport feature in Intellij and no one cares which one is imported :)

ripcurlx · 2019-11-22T09:58:02Z

@bodymindarts It looks like you still left the unused imports in and a small typo in the commit message @nAmed 😉

bodymindarts · 2019-11-22T09:58:05Z

I have force pushed removing the annotation from the abstract classes.

I have consistently used

import javax.inject.Named;

To mirror the usage of

import javax.inject.Inject;

These are library agnostic and should still be effective if a different DI framework is used. I didn't notice that another import was used elsewhere.

ripcurlx · 2019-11-22T09:59:14Z

I have force pushed removing the annotation from the abstract classes.

I have consistently used
import javax.inject.Named;
To mirror the usage of
import javax.inject.Inject;
These are library agnostic and should still be effective if a different DI framework is used. I didn't notice that another import was used elsewhere.

I guess we should get rid of the Guice bound annotations in favor of the generic one at some point.

bodymindarts · 2019-11-22T10:02:02Z

Removed the unused imports

bodymindarts · 2019-11-22T10:04:02Z

And commit message is no typo. If you look at the message locally its fine. It seems to be a display bug in github.

ripcurlx · 2019-11-22T10:05:10Z

It seems to be a display bug in github.

Yes it looks like GitHub tries to be clever finding a user with that name 🤦‍♂

ripcurlx

ACK

lusarz · 2019-11-22T11:58:53Z

@bodymindarts Would you prepare pull request that removes unnecessary @FxmlView annotations from abstract views ? You should be able to find them using @FxmlView[\s\n]*public abstract regex.

bodymindarts · 2019-11-22T13:57:27Z

@lusarz #3661

bodymindarts requested review from ripcurlx and sqrrm as code owners November 21, 2019 22:00

lusarz suggested changes Nov 22, 2019

View reviewed changes

ripcurlx suggested changes Nov 22, 2019

View reviewed changes

bodymindarts force-pushed the missing-annotations branch from 7a92f63 to 90f523a Compare November 22, 2019 09:56

bodymindarts force-pushed the missing-annotations branch from 90f523a to 40d0376 Compare November 22, 2019 10:01

Add missing @Named annotations for CoinFormatter injection

88d031f

bodymindarts force-pushed the missing-annotations branch from 40d0376 to 88d031f Compare November 22, 2019 10:02

ripcurlx approved these changes Nov 22, 2019

View reviewed changes

ripcurlx merged commit 90bf138 into bisq-network:master Nov 22, 2019

bodymindarts deleted the missing-annotations branch November 22, 2019 13:57

lusarz mentioned this pull request Nov 29, 2019

For Cycle 8 bisq-network/compensation#418

Closed

bodymindarts mentioned this pull request Dec 11, 2019

For Cycle 8 bisq-network/compensation#428

Closed

stejbac mentioned this pull request Dec 13, 2019

For Cycle 8 bisq-network/compensation#436

Closed

ripcurlx mentioned this pull request Dec 13, 2019

For Cycle 8 bisq-network/compensation#439

Closed

stejbac mentioned this pull request Jan 12, 2020

For Cycle 9 bisq-network/compensation#459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing @Named annotations for CoinFormatter injection #3658

Add missing @Named annotations for CoinFormatter injection #3658

bodymindarts commented Nov 21, 2019 •

edited

Loading

lusarz Nov 22, 2019

bodymindarts Nov 22, 2019

lusarz Nov 22, 2019

stejbac Nov 22, 2019

ripcurlx Nov 22, 2019

ripcurlx Nov 22, 2019

lusarz Nov 22, 2019

chimp1984 Nov 24, 2019

chimp1984 Nov 24, 2019

ripcurlx left a comment

ripcurlx Nov 22, 2019

ripcurlx Nov 22, 2019

ripcurlx Nov 22, 2019

ripcurlx commented Nov 22, 2019

ripcurlx commented Nov 22, 2019

lusarz commented Nov 22, 2019

ripcurlx commented Nov 22, 2019

bodymindarts commented Nov 22, 2019

ripcurlx commented Nov 22, 2019

bodymindarts commented Nov 22, 2019

bodymindarts commented Nov 22, 2019

ripcurlx commented Nov 22, 2019

ripcurlx left a comment

lusarz commented Nov 22, 2019

bodymindarts commented Nov 22, 2019

Add missing @Named annotations for CoinFormatter injection #3658

Add missing @Named annotations for CoinFormatter injection #3658

Conversation

bodymindarts commented Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ripcurlx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ripcurlx commented Nov 22, 2019

ripcurlx commented Nov 22, 2019

lusarz commented Nov 22, 2019

ripcurlx commented Nov 22, 2019

bodymindarts commented Nov 22, 2019

ripcurlx commented Nov 22, 2019

bodymindarts commented Nov 22, 2019

bodymindarts commented Nov 22, 2019

ripcurlx commented Nov 22, 2019

ripcurlx left a comment

Choose a reason for hiding this comment

lusarz commented Nov 22, 2019

bodymindarts commented Nov 22, 2019

bodymindarts commented Nov 21, 2019 •

edited

Loading