search

ho-dev / HattrickOrganizer

Assistant for Hattrick online football manager
https://ho-dev.github.io/HattrickOrganizer/
GNU Lesser General Public License v3.0
190 stars 77 forks source link

#752 LineupPlayerTable uses TableModelListener instead of MouseListener #2049

Closed wsbrenk closed 2 months ago

wsbrenk commented 3 months ago

  1. changes proposed in this pull request:

    • fixes issue #752
    • if not fixing an existing issue, comments explaining the purpose of the PR should be provided)

  2. src/main/resources/release_notes.md ...

    • [ ] has been updated
    • [x] does not require update

  3. [Optional] suggested person to review this PR @tychobrailleur

wsbrenk commented 3 months ago

@tychobrailleur I really have no idea if a TableModelListener is better than a MouseListener. So I need your advice.

In version 9 we could really think about whether we can bring the confusingly many different tables of the HO to a common (Kotlin) solution.

tychobrailleur commented 3 months ago

@tychobrailleur I really have no idea if a TableModelListener is better than a MouseListener. So I need your advice.

For sorting, a MouseListener would be more appropriate, as it is invoked in reaction to a click. A TableModelListener would react to any change to the model backing the table, so it could be anything, even things other than a click.

If I remember correctly, sorting in HO is done in a very custom way, which doesn't use the JTable facilities (i.e. TableRowSorter), but I think this is because of the very complex things like the split table players' name / rest of attributes.

What problem are you trying to solve here?

In version 9 we could really think about whether we can bring the confusingly many different tables of the HO to a common (Kotlin) solution.

Yes, agreed, we could do with a bit of consistency, and unify into a generic component that would make maintenance a bit easier!

wsbrenk commented 3 months ago

@tychobrailleur In https://github.com/ho-dev/HattrickOrganizer/issues/744#issuecomment-715419003 Jannis recommends using a tableModelListener - so i tried my very best, but i'm not sure if it is a better solution than the previous.

tychobrailleur commented 3 months ago

@tychobrailleur In #744 (comment) Jannis recommends using a tableModelListener - so i tried my very best, but i'm not sure if it is a better solution than the previous.

Ah fair enough, if the expert says so! ;-)

Let me have a closer look, then... But I sympathise with your “Widerstreben”!

tychobrailleur commented 2 months ago

Sorry for the delay, I'm looking into this tonight.

tychobrailleur commented 2 months ago

I have had a closer a look at the thread at #744 , and I think it makes sense to use a TableModelListener here: I had not fully grasped that the click was not a click to act on the table itself (for sorting), but for a value in the table (the state of that unselectable feature). So the way you have it is good.

One thing I am not sure is the behaviour of the Best Position feature. In the Squad tab, I pick two consecutive players and mark them both as Keeper in the override, but only one appears as manually overridden in the Lineup tab

optimized

Probably a separate issue...

wsbrenk commented 2 months ago

Probably a separate issue...

i can not reproduce this - Are you sure you selected the same player?

wsbrenk commented 2 months ago

Now i think i see, what you were doing. The second player you selected is your trainer i think. This player is not displayed in the lineup panel if he can no longer play in the team.

tychobrailleur commented 2 months ago

Now i think i see, what you were doing. The second player you selected is your trainer i think. This player is not displayed in the lineup panel if he can no longer play in the team.

Should he appear in the table of players in the Lineup tab, then, if that's the case? Or should we make it “unselectable“ by default, without a way to override?