Code review comment for lp:~mzanetti/unity-api/launcher-api-pinning

W dniu 05.07.2013 17:23, Michael Zanetti pisze:
>> 116 Q_INVOKABLE virtual void move(int oldIndex, int newIndex) = 0;
>>
>> 131 + Q_INVOKABLE virtual void pin(int index) = 0;
>>
>> 142 + Q_INVOKABLE virtual void remove(int index) = 0;
>>
>> 151 + Q_INVOKABLE virtual void triggerQuickListAction(int itemIndex, int
>> quickListIndex) = 0;
>>
>> I wonder if we should instead identify the items by their ids and not indexes?
>> Maybe not for the move(), but for pin() / unpin() and trigger() I would.
>
> So far this API does not know anything about id's. Identifying items by ID would require to introduce some ID in QML which I intended not to do so far.
>
>> pin() should also take the index at which to pin the item (when you drag an
>> item onto the launcher from the dash).
>
> Would be a possibility too I guess. I did like the idea of not having to deal with app ids in QML but I'm not strongly against it either.
>
> Still think I should change it? If yes, I'll do.

I just think that that _is_ the identifier. The index is not, not
really. So yeah, I'd rather deal with app IDs I think.

>> Not sure if remove should not be
>> "unpin", exactly because it might not actually result in the removal of the
>> item - it's the backend model that decides what happens.
>
> which would then require another remove() method because recent apps are not pinned. I for one would prefer keeping this single method that works for all items instead of 2 different ones that only work for some items.

Do we even support removing recent apps? But ok, I'm good with that.
Just make it explicit in the comment (and maybe rename the method to
something suggesting that - requestRemove()) that this is just a request
that isn't necessarily executed straight away.

>> 206 + * - RoleLabel: The text entry in the QuickList menu (QString).
>> 207 + * - RoleIcon: The icon to be shown for this entry (QString).
>> 208 + * - RoleCheckable: Wether the item is checkable or not (bool).
>> 209 + * - RoleGroup: A group this item is in. Items in the same group are
>> exclusive (int). -1 for no group.
>>
>> 220 + enum Roles {
>> 221 + RoleLabel,
>> 222 + RoleIcon
>> 223 + };
>>
>> Hmm does it?
>>
>> Shouldn't that be "label", "icon", "checkable", "group", too, instead?
>
> Right... my draft had that stuff already in, but Antti asked me to leave it away because grouping and checking is not planned for the October deadline. I removed it from the comments too now.
>
>> "The icon" - should that be full URL or only a themed icon name?
>
> I don't know yet... depends a bit on what the backend will deliver. However, the API doesn't really care. In the end its a string that must be resolvable. I guess if the backend decides to mix them for some reason it should be fine for us too.
>
>>
>> "checkable" - can you reword?
>
> I used checkable because that's what Qt uses for everything that is checkable. Anyways, its gone from the API for now.

OK.

>> 253 -add_subdirectory(copyright)
>> 254 -add_subdirectory(whitespace)
>> 255 +#add_subdirectory(copyright)
>> 256 +#add_subdirectory(whitespace)
>>
>> Oh no you don't! ;P
>
> Ooops. Fixed. :D
>
> Btw. those are failing in pbuilder because they parse the the build directory. I wonder actually how they build in Jenkins...

That's because your _current_ dir is packaged whole. It's not the
jenkins builddir, it's your own builddir that it fails on. Use bzr bd to
work on stuff that's actually checked in.

--
Michał (Saviq) Sawicz <email address hidden>
Canonical Services Ltd.