Selection drawing error in list

[humdinger]

This is hrev32931.

• Select a partition from the list.
• Click on another partition in the graphical repesentation above.

-> See the first selected partition still having a darker border around its row. See screenshot.

[humdinger]

dark border around first selected partition

[stippi]

This is a BColumnListView pecularity. The current drawing is intentional and shows the keyboard focused row. But IMHO, the keyboard focused row should just change with the mouse selection.

[Janus]

An ugly solution. Does it worth fix it?

[Janus]

Typo (BRow* focusRow)

[stippi]

This is not a fix, and it makes things worse, actually, by not adopting the UI to the selection under a mysterious condition.

See my comment:1. The fix should be done in BColumnListView. The focus row is set when the user clicks a row (and it is selected by the list view code). However, when another row is selected programatically, the focus row is not adjusted to the changed selection. However, that is what should happen.

[Janus]

Replying to stippi:

This is not a fix, and it makes things worse, actually, by not adopting the UI to the selection under a mysterious condition.

I don't understand what do you mean, this patch move the focus row with the selection when the user select one partition in the PartitionView.

How make things worse?

See my comment:1. The fix should be done in BColumnListView. The focus row is set when the user clicks a row (and it is selected by the list view code). However, when another row is selected programatically, the focus row is not adjusted to the changed selection. However, that is what should happen.

This works in single row selection, with SetFocusRow you move the focus and eventually the selection. But this function invoke the SELECTION_CHANGE only if the focusRow is different from the previous value.

I agree that the class need a review, the edit mode doesn't work at all.

[stippi]

Replying to Janus:

Replying to stippi:

This is not a fix, and it makes things worse, actually, by not adopting the UI to the selection under a mysterious condition.

I don't understand what do you mean, this patch move the focus row with the selection when the user select one partition in the PartitionView.

But in the wrong place. Your patch is for DriveSetup, but any patch of this kind should be in BColumnListView. If it is the desired behavior in DriveSetup to move the focus row with the selection, then it must be the desired behavior in any other app. But your patch can obviously not fix it for other applications, unless you fix it at the source of the problem.

How make things worse?

You introduced a condition if (focusRow != row). If this condition is true, then _AdaptToSelectedPartition() no longer called. I am pretty convinced that this is a real error. The UI will not adopt to the selected partition anymore, unless the focus row already happens to be the selected partition.

See my comment:1. The fix should be done in BColumnListView. The focus row is set when the user clicks a row (and it is selected by the list view code). However, when another row is selected programatically, the focus row is not adjusted to the changed selection. However, that is what should happen.

This works in single row selection, with SetFocusRow you move the focus and eventually the selection. But this function invoke the SELECTION_CHANGE only if the focusRow is different from the previous value.

Ok, but I am talking about these calls:

fListView->DeselectAll(); 
fListView->AddToSelection(row);

This should change the focus row automatically, because it makes no sense that it should stay detached from the selection.

I agree that the class need a review, the edit mode doesn't work at all.

This seems unrelated.

[Janus]

You introduced a condition if (focusRow != row). If this condition is true, then _AdaptToSelectedPartition() no longer called. I am pretty convinced that this is a real error. The UI will not adopt to the selected partition anymore, unless the focus row already happens to be the selected partition.

This is on purpose, if the focusRow is different from the previous the SetFocusRow send a Selection_change that invoke _AdaptToSelectedPartition() and all the code replicated in the two switch cases.

Ok. As you can see from my irritation, it is good to add a comment to such code. The effect is completely counter-intuitive.

In any case, the code in DriveSetup should not be changed at all. I hope I have explained why I think the change needs to be in BColumnListView. The DriveSetup code cares about the selection. The selection in the graphical view of the partitions and the selection in the list view should be in sync. The "focus row" is something that DriveSetup does not care about. It should not have to set the focus row, because that indirectly also sets the selection. The focus row is something which the list view cares about and it relates to keyboard navigation. Since it makes sense that the focus row is synced with the selection, then it is the job of BColumnListView to sync it when the selection changes. Just as it already changes the selection when someone programmatically changes the focus row.

[Janus]

Stippi this is my comment to the patch

An ugly solution. Does it worth fix it?

We are on the same page :-)

[stippi]

Replying to Janus:

Stippi this is my comment to the patch

An ugly solution. Does it worth fix it?

We are on the same page :-)

I saw that and I understand. :-) I'm trying to convince you that it is not good/worth to commit the patch.