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

> 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.

> 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.

> =====
>
> 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.

> =====
>
> 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...

> =====
>
> Please put Authors: consistently below the copyright.

fixed.

>
> =====
>
> Maybe put the SignalSpy at the top?

Done.