Opened 15 years ago

Last modified 8 years ago

#4414 new bug

Selection drawing error in list

Reported by: humdinger Owned by: nobody
Priority: low Milestone: R1
Component: Kits/Interface Kit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Platform: All

Description

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.

Attachments (2)

drivesetup-selection.png (45.1 KB ) - added by humdinger 15 years ago.
dark border around first selected partition
0001-DriveSetup-moves-focus-row-with-selection.patch (943 bytes ) - added by Janus 10 years ago.

Download all attachments as: .zip

Change History (14)

by humdinger, 15 years ago

Attachment: drivesetup-selection.png added

dark border around first selected partition

comment:1 by stippi, 15 years ago

Component: Applications/DriveSetupKits/Interface Kit
Owner: changed from stippi to axeld
Priority: normallow

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.

comment:2 by axeld, 15 years ago

Owner: changed from axeld to nobody
Version: R1/pre-alpha1R1/Development

comment:3 by Janus, 10 years ago

patch: 01

comment:4 by Janus, 10 years ago

An ugly solution. Does it worth fix it?

comment:5 by Janus, 10 years ago

Typo (BRow* focusRow)

comment:6 by stippi, 10 years ago

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.

in reply to:  6 ; comment:7 by Janus, 10 years ago

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.

in reply to:  7 ; comment:8 by stippi, 10 years ago

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.

in reply to:  8 ; comment:9 by Janus, 10 years ago

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.

Last edited 10 years ago by stippi (previous) (diff)

in reply to:  9 ; comment:10 by Janus, 10 years ago

Stippi this is my comment to the patch

An ugly solution. Does it worth fix it?

We are on the same page :-)

in reply to:  10 comment:11 by stippi, 10 years ago

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.

comment:12 by pulkomandy, 8 years ago

patch: 10
Note: See TracTickets for help on using tickets.