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)
Change History (14)
by , 15 years ago
Attachment: | drivesetup-selection.png added |
---|
comment:1 by , 15 years ago
Component: | Applications/DriveSetup → Kits/Interface Kit |
---|---|
Owner: | changed from | to
Priority: | normal → low |
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 , 15 years ago
Owner: | changed from | to
---|---|
Version: | R1/pre-alpha1 → R1/Development |
by , 10 years ago
Attachment: | 0001-DriveSetup-moves-focus-row-with-selection.patch added |
---|
comment:3 by , 10 years ago
patch: | 0 → 1 |
---|
follow-up: 7 comment:6 by , 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.
follow-up: 8 comment:7 by , 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.
follow-up: 9 comment:8 by , 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.
follow-up: 10 comment:9 by , 10 years ago
You introduced a condition
if (focusRow != row)
. If this condition istrue
, 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.
follow-up: 11 comment:10 by , 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 :-)
comment:11 by , 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 , 8 years ago
patch: | 1 → 0 |
---|
dark border around first selected partition