ho-dev
Improve layout of Team Analyzer slightly
As per conversation at #2020, this is the first pass at improving the layout of the Team Analyzer, and add some info about the team selected in the dropdown (see screenshot at #2020). I have removed the streak for now, as it requires to download more information. This PR is to get initial feedback and comments on these changes.
I have made a conscious effort of adding comments where appropriate to describe the existing Team Analyzer code, hopefully, it'll be easier to go through that code in the future. :)
-
changes proposed in this pull request:
- fixes issue #2020
-
src/main/resources/release_notes.md...
- [x] has been updated
- [ ] does not require update
- [Optional] suggested person to review this PR @wsbrenk
some minor things
Any comment/suggestion on the UI? Is the flow clear enough?
One thing I was wondering is whether we should just remove “Analyze” and trigger the analysis automatically, once the update is complete.
@tychobrailleur UI looks very good to me. Concerning triggering the automatic update i was thinking about it in the past too.
Since the update is very expansive due to a number of downloads from hattrick, which are only necessary when new matches happened, i would not invoke it on each analyze invocation. I run several analyzes each time i change my own lineup. We could make it smarter if on analyze we check if new matches happened since last analyze and start the automatic download (update) only in that case, when matches had taken place since then.
@tychobrailleur UI looks very good to me. Concerning triggering the automatic update i was thinking about it in the past too.
Since the update is very expansive due to a number of downloads from hattrick, which are only necessary when new matches happened, i would not invoke it on each analyze invocation. I run several analyzes each time i change my own lineup. We could make it smarter if on analyze we check if new matches happened since last analyze and start the automatic download (update) only in that case, when matches had taken place since then.
Ah yes, I think I vaguely remember we've had that same conversation in the past. I'll have a think about your suggestion, this definitely sounds reasonable.
When you say “expensive,” are you referring to the time it takes to make those downloads, or are you concerned about the actual number of calls made to HT? Are you concerned about the load on CHPP?
When you say “expensive,” are you referring to the time it takes to make those downloads, or are you concerned about the actual number of calls made to HT? Are you concerned about the load on CHPP?
A little bit of both. But I would be more urgent about the first point, although I had only recently noticed a stupid error that must have caused a very heavy load on the chpp machines. We should avoid becoming the focus of HTs through something like this.
We should avoid becoming the focus of HTs through something like this.
Agreed. We should remain good citizens of the Hattrick community. This is partly the intent of caching there: https://github.com/ho-dev/HattrickOrganizer/blob/679816a1e3aa6a7d123ad27774799136eb920a27/src/main/java/module/teamAnalyzer/ht/HattrickManager.java#L26
Also, noticed that we potentially download the same match details in that method: https://github.com/ho-dev/HattrickOrganizer/blob/679816a1e3aa6a7d123ad27774799136eb920a27/src/main/java/core/net/OnlineWorker.java#L247 There would be room for optimisation there, I believe.
I like caching a lot - sometimes more than it's good.
In this case we have to consider the situations when i started and updated HO before i start watching the league matches and want to analyze again, when the matches are finished. HO should be smart enough to download the new matches after they were finished.
@tychobrailleur are you still working on this issue or will you fix the conflicts and merge this pr in the next time - my approvement ist now 2 weeks old;-)
Thanks, I have now resolved the conflicts, and merged. Let's see if we get more feedback from users that way!