Universalmediaserver: Clarification request to refresh DLNAResource children

[taconaut]

I've noticed that since porting mlx from pms to ums, the refreshing of the children for mlx classes extending DLNAResource didn't work correctly anymore. The refreshing was being done by overriding refreshChildren(). Now there are four methods refreshChildren(), refreshChildren(String search), doRefreshChildren() and doRefreshChildren(String search) which can be overridden. Could somebody please explain when which of these methods should be overriden?

[SharkHunter]

The refreshChildren changed back at PMS and broke a lot of things. The doRefreshChildren was introduced back then. refreshChildren I believe is the old one ie. the one that pre-dates the PMS rewrite (which was bad). The string versions allows you do a search in your folder. If you don't support searching these methods just calls the argumentless version.

Most of the refresh is controlled via isRefreshNeeded.

[taconaut]

@SharkHunter Could the ones which are not do be marked as deprecated to avoid confusion?

[SharkHunter]

refreshChildren just calls doRefreshChildren (if isRefreshNeeded returns true) so that should most likely not be overridden but instead use isRefreshNeeded. The doRefresh is the actual work horse. The poor naming is inherited from the PMS rewrite. Before refreshChild worked like doRefresh

[taconaut]

In my case, isRefreshNeeded always returns true, but refreshChildren() is not being called. I haven't tried if overriding doRefreshChildren() works.

[SharkHunter]

Yes thats my case too and then you must override doRefreshChildren.

[taconaut]

That's what I suspected :) I might submit a pull request with javadocs for those methods when having dug a bit deeper into the subject.

[taconaut]

The easiest way to show the calls to those methods is to see the incoming calls (as shown with eclipse):

refreshChildren() is only called from RootFolder.scan(DLNAResource resource), which is only being triggered by a library scan. This means classes extending DLNAResource wanting to offer refreshing functionality have to override one of the do refreshing methods.

[SharkHunter]

Or use the isRefreshNeeded function

[taconaut]

Do you mean do the actual refreshing in isRefreshNeeded? I always used it to return true/false depending if I wanted refreshChildren() to be called (this is how it worked at some point).

[SharkHunter]

Well yes this is a mess. I blame the PMS rewrite that was a made stuff messy. The deal as it was before was to override refreshChildren and return true if you needed to refresh. Now this (seems) splitted into isrefresh which is called to determine if we should refresh and then the refresh/dorefresh pair. The dorefresh corresponds best to the old refresh while the refresh is some odd ball. Note that we changed the index system and how objects are found since so this whole refresh stuff might not be needed any more. So in general isrefresh is called first and if it returns true dorefresh will be called onway or the other. It's safe to return true always from isrefresh or you could have some logic there.

[taconaut]

Agreed, it's a mess. I've never noticed refresh issues before porting mlx to UMS though. As I know how it works now, it's easy enough to adapt the code. Removing or renaming any of those methods would very likely break existing plugins, thus they're going to stay as they are :)
Thanks for helping clarify this.

[Nadahar]

@UniversalMediaServer/developers @SubJunk
I'd say that keeping mess like this is a bad idea and ideally it shoud be cleaned up. I don't know enough about it to make a suggested cleanup though. Breaking plugins or not, having a very messy logic breaks plugins as well as the plugin author is unlikely to get it right. Impacts from minor changes like this to plugins should be considered, but not necessarily be a deal-breaker imo. If else, every mistake ever made is set in stone and we can only dig ourselves deeper into a hole.

At the very least, the logic should be properly documented in the javadocs/comments. Anything else is asking for trouble.

[SharkHunter]

@Nadahar - you never heard of backwards compatibility? And yes breaking plugins is a bad habit. Cleaning up code that works just for the sake of making it "look good" is bad. Stuff that works should NEVER be broken on purpose. This has been "cleanup" to the mess we now have. Changing it is a massive change that would rewrite the whole of UMS plus ALL plugins meaning that no plugin would work for some releases. That is bad on all accounts.

[taconaut]

Just for the record, I suppose many plugins still override refreshChildren (@skeptical e.g. jumpy) which doesn't work anymore as it's never being called (except for a library scan).

[edit] I rewoke my statemant, it's not an override of the DLNAResource method.
[edit2] Checking several plugins (jumpy, ums-Kodi, MovieInfo, Channel, GMPMS) I only found one override of refreshChildren in Subsonic.

[taconaut]

I reopen the issue as there are still discussions going on

[Nadahar]

you never heard of backwards compatibility?

I've heard of being backwards - are the two connected? It seem to me like you don't bother to consider what I write and only put on the same broken record. I've never said that backwards compatibility isn't important, but you have to look at pros and cons - you can't simply have a fixed stance. Or, I guess you can, but it doesn't make any sense as I see it.

And yes breaking plugins is a bad habit.

I disagree. Breaking plugins _can_ be bad, but I think the importance of this is being overplayed enormously here. As I said above, pros and cons have to be evaluated. They way I think is: What's the impact on implicated plugins? Is it a 2 minute fix to make them work? What's the impact on UMS not to do it? What's likly impact of the future limitations, frustrations and confusions from not cleaning up past sins. How many and which of the plugins are actually relevant, are actually being used? How many of those being relevant is beyond our control so we can't mend it?

It seems to me that having plugin authors on the development team should be a strength, it should be easy to adapt the plugins when changes were needed. Instead, it seems to have to opposite effect here. Every improvement is frawned upon simply because it could cause the plugin author the slightest amount of work.

Cleaning up code that works just for the sake of making it "look good" is bad.

I've never advocated changes to make something "look good". I don't consider readability, structure, logic and documentation "good looks" but important prerequisites for a software to stand the test of time. Yes, you can get away with ignoring this if you have a short time perspective and you're fine with limiting further development to a very small group of people. If you want something to be robust and live on even after the current author(s) abandon it, you can't. It has nothing to do with "looking good".

Stuff that works should NEVER be broken on purpose.

Who has talked about breaking things on purpose? There's a difference between changing things and braking things, but the closer to complete chaos the current state is the more similar the two can appear.

This has been "cleanup" to the mess we now have. Changing it is a massive change that would rewrite the whole of UMS plus ALL plugins meaning that no plugin would work for some releases. That is bad on all accounts.

I don know if it's true that ALL plugins pose as DLNAResources but I doubt it. How would e.g DebugPacker be broken by changing this? That said, I agree that many would be impacted. As I stated above, I currently don't have enough overview over how this works and what would be a good solution. This knowledge and a proper consideration of all consequences and use cases is a necessity if any such "cleanup" or restructuring/refactoring should succeed. The fact that someone has done a bad job at this before proves that they did a bad job, not that it can't be done.

It's like saying that if someone built a bridge badly and it fell down, we should stop building bridges. I disagree. We should build better bridges.

When it comes to no plugins working for some releases is simply not true. As we have authors for several plugins on the house, those plugins could be changed in sync. Other authors could be informed if we know about them. The only plugins likely to suffer are the ones we don't know about. How many users does that impact? That's the real impact of making such a change, that and a few minutes of time from the known plugin authors. That's what should be put in the "cons" basket.

I think making a separate "plugin API changelog" would be a good idea, where any change thought to impact plugins (it's a guesswork after all since everything is open and nothing is documented) would be documented. In that way it would be easy for any plugin writer to see all that's changed since the last time the plugin was maintained, and quickly see what impact that would have on that particular plugins and what's the resolution. It's very much easier to adapt to a change when the changes are described.

[SubJunk]

If there have been "cleanup" changes in the past that you guys don't like, we can try rolling them back. I agree there were a lot of PMS refactoring commits just before that project went dead and it might have been a mistake for us to take some of those updates for UMS.

[polygirl]

@taconaut No further discussion for some time on this stalled issue, so closing. If you'd like to reopen it, please do.