[PATCH] enable external drives list in kickoff again

[Aaron J. Seigo]

On Thursday 27 March 2008, Marco Martin wrote:
> Hi,
> i was trying to enable the listing of removable media in kickoff again
> the problem was that SystemModel::rowCount() returned always 0

huzzah =) note that these models are a bit. ... interesting .. in a few places 
and so can be tricky to work with at times.

> case REMOVABLE_ROW:
>   return d->numDevices;
> seems not to work, but it works with
> case REMOVABLE_ROW:
>   return d->placesModel->rowCount();
> that i feel is well, wrong?

yes, it would return all the places model entries. so ... 

first, the BookmarksRow right now returns d->placesModel->rowCount() which i 
don't think is right. it should probably be, in your patch, 
d->placesModel->rowCount() - d->numDevices otherwise it will list every row 
in there. i don't think that's related to your problem, in any case.

as for why the problem exists.. here's my guess from looking at the code: the 
problem is in QVariant SystemModel::data(const QModelIndex &index, int role) 
const.

in it, it maps the index of the item being requested using mapToSource which 
simply does:

d->placesModel->index(proxyIndex.row(), proxyIndex.column())

so if d->numDevices isn't large enough to index the removable items then it 
won't get back a removable item. so let's say that the places model contains 
the following: "home folder, folder 'a', folder 'b', removable usb disk. 
d->numDevices will be  1 and that will return "home folder". 

back in SystemModel::data, it then checks for:

if (!isDevice && parent.row()==BOOKMARKS_ROW) {

which fails (wrong row, but it's not a device), but the else fails as well:

} else if (isDevice) {

and so neither branch is followed, wellPlaced == false and it returns a 
QVariant() (or, invalid/empty data). 

the buzzer sounds and it doesn't work.

when you return  d->placesModel->rowCount(); though it ends up iterating 
through the whole placesModel, discarding the bad entries (e.g. "Home 
Directory" under the REMOVABLE_ROW) and showing the good ones.

so while this would probably screw up any "real" view (i'll bet it shows empty 
rows in a qtreeview, for instance) , it works with kickoff's views and that's 
probably all that matters. it may be work documenting this ...

alternatively, one *could* make the view well behaved and actually map, e.g. 
via a proxy model or some such (or even just really go the cheap route and 
have a couple of index maps around), the items which are bookmarks or devices 
in the places model.

i think that this would be unfortunate wasted effort, even if it does make the 
model more correct, until the point when we have a view that uses this that 
is willing to show rows that have QVariant() data.

iow, just go with  d->placesModel->rowCount(); even though you are right that 
this is not exactly Correct, though it works.

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20080327/1d10c429/attachment.pgp