Make AbstractNamedBean.toString() final

[danielb987]

The purpose of this PR is to show the consequences of making AbstractNamedBean.toString() final.

Most of the toString() methods seem to be removable, but these may cause problems:
jmri.implementation.DefaultRailCom
jmri.implementation.DefaultSignalSystem
jmri.jmrit.logix.Portal

[pabender]

I really think making toString final for namedbeans is wrong. The JavaDoc for ToString states:

In general, the toString method returns a string that "textually represents" this object.

And

It is recommended that all subclasses override this method.

( see
https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#toString-- )

You can’t textually represent an object if there is a final method that object is intended to use when the object details differ.

The bottom line is this PR violates the basic understanding of toString for objects, especially the part noting that all subclasses should provide an implementation.

In the specific case of IDTags and RailCom objects, the toString is the report presented by the associated reporters on both panels and in the table. By design, reporters report objects and use the toString method when asked for a the report.

[pabender]

I recommend this pr be closed as invalid.

[danielb987]

@pabender
I'm fine with that, but in that case the javadoc should be updated to reflect that. Currently AbstractNamedBean.toString() states It would be good to eventually make this final to keep it consistent system-wide and some of the implementing classes states that they violates the contract in NamedBean.toString().

On the other hand, the javadoc of NamedBean.toString() states that toString() Display the system-specific name. So the contract of NamedBean says that toString() should return the system name. If this is wrong, the javadoc for NamedBean.toString() should be updated as well.

So we have a conflict between the contract of Object.toString() and NamedBean.toString(). If Object.toString() should rule, I can create a new PR that changes the javadoc for NamedBean.toString and the affected classes that are mention in this PR.

[bobjacobsen]

In the specific case of IDTags and RailCom objects, the toString is the report presented by the associated reporters on both panels and in the table. By design, reporters report objects and use the toString method when asked for a the report.

This part gets tricky. `toString` is specified as representing an _object_ not the state of that object. A lot of infrastructure code, e.g. in Swing, assumes the result of `toString` is invariant.

[pabender]

Hmmm...

On the one hand, we can be more restrictive than Object.toString’s contract.

On the other hand, At one point in time, a system prefix and type letter could precisely identify an object. Since system prefixes are now arbitrary, using the system name as the output of toString provides little information in the absence of other context.

That said, my biggest issue with this change is it breaks the behavior of all of our reporters that report IDTag or RailCom objects.

If we take the system name of an RFID tag ( which is represented using an IdTag object ) as an example, the system name for the tags most of us use is based on the 20 ASCII character random value encoded into the tag itself. Displaying the system name in this case is absolutely meaningless because we have no idea what that tag is representing.

Unless we change the contract of the Reporter so that it reports something more restrictive than Object, the toString Methods for IDTags and RailCom objects will need to remain the same. Changing the contract for Reporters WILL require changes to LocoNet reporters, which currently report String objects.

I don’t have a strong opinion on how this impacts other named bean types.

[pabender]

In the specific case of IDTags and RailCom objects, the toString is the report presented by the associated reporters on both panels and in the table. By design, reporters report objects and use the toString method when asked for a the report.
This part gets tricky. toString is specified as representing an object not the state of that object. A lot of infrastructure code, e.g. in Swing, assumes the result of toString is invariant.

I don’t disagree, and this PR does have me thinking about that.

Looking at IDTag objects, toString currently ( before this PR ) provides either the tag ID or the Username. While the user name can be set, it is unlikely to change.

RailCom objects are a bit more problematic. They report the address along with state data. More appropriately, the RailCom objects should follow the example of the IDTags and report a user name, if it exists, or the address ( which is by definition a DCC address, though it is not required to be a mobile decoder address ( accessory decoders can implement RailCom too, though I don’t know of any that do ).

Most of what is currently in the RailCom toString method should probably be in the describeState method ( though most of the data doesn’t pertain to the seen or unseen values.)

The LocoNet reporters do need to report an IDTag based object however. We can’t directly use the LocoNet reports in Operations without that.

[bobjacobsen]

@pabender Although I agree that user names "unlikely to change", @KenC57 has been making a pretty good case that there are cases where that change is valuable.

There are two kinds of problems with allowing beans to make run-time changes in the result of toString()

• (bad) the change is so large that it results in changes to the sort order of items in tables, trees, JComboBoxes and more generally the sorted Collections. In the Swing case, this is generally disastrous: you can select the wrong item, get ConcurrentModificationExceptions and just generally have Bad Things happen. We really need to avoid this.

• (not so bad) But if the sort order is not affected, as far as I know the only issue is the possibility of stale content: An item was filled out from an earlier toString, but it's the same item.

You get that second case if the toString value is e.g. getSystemName()+" ("+getUserName()+")" or similar where the getSystemName() at the front provides enough for a sort. (There's still an issue over parsable, which includes that sub-case of sortable, system names, but we're making progress consistent on that)

Type II has some additional issues in our existing code, where a lot of stuff is still passing around a String name instead of a bean references or a NamedBeanReference. But in the long term we need to make progress on that in any case, so I don't view that as an issue that would prevent a type-II toString.

Bottom line: I think we could standardize on permitting user name info in the NamedBean implementation of toString(). It's not formally correct, but maybe the case can be made that it's OK if proceeded by the system name. I think that additional state information could also be OK (again preceded by system name), but that's pretty far from the common definition, and I'd really like for somebody to make an argument as to why that's a necessary thing to include.

[pabender]

@bobjacobsen I am not arguing that the current implementation of toString in IDTag and RailCom objects is correct.

What I am arguing is that the current implementation of reporters has forced the current behavior on us to get the result we want.

That particular result is that we want the user ID of the tag ( which typically is a reporting mark for a piece of equipment ) to appear in the report. The IDTag implementation of toString does this by providing the userid or the tag ID. This is all ok in the sense that unless there is an error, you don’t tend to change the reporting marks. State information is never returned by the toString method.

The RailCom implementation of toString incorrectly provides state information from the report followed by the DCC address from the report. The address itself is OK, but it really should at least follow the same pattern as the IDTags. ( I did not write either of these implementations and have just lived with it to now ).

I am going to take on some work to fix a few issues in the Reporter mechanisms. Here are the issues I want to address now:

First, we have Operations setup so that when an IDTag associated with a piece of rolling stock appears at a reporter, and the reporter is associated with a location, the location of that piece of rolling stock is updated to to match. This works with both IDTags and RailCom objects ( which are derived from IDTags ). It doesn’t work when the reporter reports a string object ( which is the case for LocoNet, RPS, and JMRIClient reporters ) so we need to look into fixing that when possible by changing the report types of the reporter to be something derived from IDTag.

Second, at least when the report is of a type we can control, the report text shown in a reporter on a panel or in the reporter table should not be the toString of the object, but a designated report string. My thought is perhaps we should add a new reportable interface that defines a toReportString method. We can then rewrite the reporter table code and the on panel reporter icons to use this method if available but use toString otherwise ( maintaining backward compatibility).

I will work on the second issue first. Once this code is in place, IDTag and RailCom objects should be able to use a common toString.

[bobjacobsen]

@pabender I don't understand all the ramifications, but what you describe sounds right to me. Thank you.

Returning to the question of this PR, perhaps there's another approach (haven't tried this, might be subtle compiler issues).

• We could provide a default implementation (which can't be final, but doesn't need to be) that's getSystemName()+" ("+getUserName()+")" (just getSystemName() would be fine too) that's annotated with @OverridingMethodsMustInvokeSuper and is documented to say that the super.toString() content has to be the first part of anything that a sub-class returns.

• We don't have any static checks that enforce @OverridingMethodsMustInvokeSuper(yet), but we could manually go through and make it true.

• Then, if we get artifacts because e.g. something is storing toString() instead of a NamedBeanreference (i.e. there's a problem when a user name is changed), we just fix those in the appropriate way.

I think there is some value in at least getting to a clearly-documented understanding that toString() has to start with system name.

[danielb987]

See PR #6464 for a possible solution on the LocoNet problem.

[danielb987]

Due to jmri.jmrix.loconet.TranspondingTag.toString() is marked depricated but will be kept for maybe two years, this PR will not be able to be completed until then.

[pabender]

@danielb987 a minor correction to my earlier statement, our documentation says we keep deprecated things used in scripts around for at least two production releases:

http://jmri.org/help/en/html/doc/Technical/RP.shtml

So a year from now we can remove that.

(and yes, I still need to update the sample scripts that use this)

[codecov]

Codecov Report

Merging #6463 into master will increase coverage by 0.1%.
The diff coverage is 39.08%.

@@             Coverage Diff             @@
##             master    #6463     +/-   ##
===========================================
+ Coverage     47.43%   47.53%   +0.1%     
- Complexity    51339    51806    +467     
===========================================
  Files          4178     4199     +21     
  Lines        402254   405074   +2820     
  Branches      59926    60808    +882     
===========================================
+ Hits         190790   192540   +1750     
- Misses       191996   192912    +916     
- Partials      19468    19622    +154

Impacted Files	Coverage Δ	Complexity Δ
...can/cbus/swing/nodeconfig/CbusNodeBackupsPane.java	56% <ø> (ø)	16 <0> (ø)	⬇️
.../jmrix/can/cbus/swing/CbusEventHighlightPanel.java	64.12% <ø> (ø)	1 <0> (ø)	⬇️
java/src/jmri/jmrix/can/cbus/swing/CbusMenu.java	73.33% <ø> (ø)	2 <0> (ø)	⬇️
...va/src/jmri/jmrit/vsdecoder/ConfigurableSound.java	5.08% <ø> (-0.33%)	2 <0> (ø)
java/src/jmri/jmrix/can/cbus/CbusConstants.java	100% <ø> (ø)	1 <0> (ø)	⬇️
...ava/src/jmri/jmrit/vsdecoder/VSDecoderManager.java	14.22% <ø> (ø)	14 <0> (ø)	⬇️
...pters/gridconnect/canrs/MergTrafficController.java	19.04% <0%> (-0.96%)	2 <0> (ø)
java/src/jmri/jmrit/vsdecoder/VSDecoder.java	6.06% <0%> (+0.31%)	3 <0> (ø)	⬇️
...adapters/gridconnect/net/NetworkDriverAdapter.java	26.31% <0%> (-5.95%)	3 <0> (ø)
java/src/jmri/jmrit/display/SignalHeadIcon.java	24.4% <0%> (ø)	28 <0> (ø)	⬇️
... and 95 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fa17d0...eb9e9af. Read the comment docs.

[bobjacobsen]

To me, this PR seems to be more about "have a contract on the content of NamedBean#toString()" rather than about it being final.

The general Object#toString() contract doesn't really help here:

Returns a string representation of the object.

The literature shows that there are two basic opinions for what toString should provide: The identity of the object, or the content.

• The default toString in the Object class is clearly providing the identity: It shows the address of the object.
• Many complex Java-provided objects, particularly in Swing, provide the content: They show a long list of values of internal variables

The middle ground is populated a little: Some Swing components expect that toString will provide the string that should be shown in the GUI, for example, which probable is a representation of just some of the content in a nicely-formatted form.

1. I think the right thing for toString() of a NamedBean to return is it's SystemName. If somebody were to give a good reason for some subclass to do more than that, that's fine, I just can't think of one now. But I think there should be a strong convention that the SystemName is always there.

2. Randall has a good point that Java itself doesn't provide a way to insist that an implementation in NamedBean itself is final (default methods can't be final). If that's an important thing to accomplish, we could move the common toString implementation up to NamedBean, add a strong comment, and add an architecture check that ensures that nothing in our code overrides it. Alternately, we can leave it in AbstractNamedBean as final, and then add a check that insists that any implementation of NamedBean goes through that. Either way means anybody else is a bit on their own, but that's always true.

[danielb987]

Following @bobjacobsen comments:
This PR now does (1) by making AbstractNamedBean.toString() final and return system name for any NamedBean that extend AbstractNamedBean.

If someone instead wants (2), it would be easy to move toString() to NamedBean and add the architecture check after this PR is merged.

So this PR is ready to be merged.

[bobjacobsen]

@danielb987 Thank you for your work on this!

I'm going to close/reopen to get a clean set of CI runs. Once that runs clean, we can merge.

[geowar1]

If .toString can only return the system name then why have two API's? If I want the system name I'll ask for it. The .toString method should NEVER be used for GUI (explicit methods /formatting, etc. should always be used instead). So that pretty much leaves .toString for debugging use. When I mouse over a variable I expect to see the .toString value (including any variable state that the .toString developer for that class felt compelled to include). There is no contract (implicit or otherwise) that says .toString can't include variable state and a lot of implementations that do exactly that. IMHO AbstractNamedBean should include the system name AND the .toString of the object that it's the named bean of; and that about it.

[danielb987]

There are 11 classes there the method toString() is removed by this PR. 7 of these belongs to Audio. Two other is DefaultSignalSystem and TranspondingTag.

The last two is AbstractAudio and AbstractStringIO which only returns the name of the class and the system name, and therefore doesn't give any more value than the current implementation.

So there is very little use of toString() as a debug tool inside JMRI. For AudioSource, I have replaced the method toString() with the method getDebugString().

[rhwood]

So there is very little use of toString() as a debug tool inside JMRI. For AudioSource, I have replaced the method toString() with the method getDebugString().

toString() is used in every log.debug(...) call in JMRI that includes the String representation of objects--its used extensively for debugging within JMRI.

[danielb987]

What I meant was that there is only 11 classes that provides their own implementation. And two of them doesn't do any useful, they only return name of class + system name.

Besides Audio, the only two other classes that provides any valuable additional information is DefaultSignalSystem and TranspondingTag.

[pabender]

On Jan 19, 2020, at 9:37 PM, Daniel ***@***.***> wrote: Besides Audio, the only two other classes that provides any valuable additional information is DefaultSignalSystem and TranspondingTag.

The TranspondingTag toString method was implemented the way it was for compatibility with scripts written with the prior Transponding implementation, which returned a string. It was deprecated upon addition to give users time to transition to using the toReportString method of the reportable interface instead. Ultimately, I do want the toString for all the IdTag objects to use the same implementation, so removing the special case from TranspondingTag at this point is fine.

[geowar1]

What I meant was that there is only 11 classes that provides their own implementation.

cd java/src
find . -type f -name '*.java' -exec grep -li 'String toString' {} ;|wc -l
189

getDebugString

Will that be used instead of toString when mousing over variables in NetBeans?

[rhwood]

No, NetBeans cannot use getDebugString() when mousing over variables because that method is not part of the Java API.

[danielb987]

CI has now completed successfully.

[danielb987]

This PR is ready to be merged. AppVeyor fails, but that is probably not related to this PR.

[bobjacobsen]

Reran tests: Travis clear, AppVeyor two different well-known intermittent failures