Code review comment for lp:~ken-vandine/content-hub/pasteboard

> In include/com/ubuntu/content/hub.h:
>
> """
> Q_INVOKABLE virtual Paste* create_paste(const QMimeData& data);
> """
>
> Who owns the created Paste object? Is the caller free to delete it? Will that
> Paste object live forever, as long as Hub does? Please document it (I know
> other methods are undocumented and that sucks).

Caller is free to delete it. The object will be available as long as the hub is running. Eventually we'll cache them to disk as well, so the stack will be restored.

>
> --------------------------------------------------
>
> In include/com/ubuntu/content/paste.h:
>
> Since Paste is public API, I would like to see it documented. Again, I know
> existing the API isn't, but we can do better than that.

Paste is technically a public API, but applications won't have direct access to the Paste object under confinement. Which reminds me why create_paste shouldn't return the Paste object. That should really just return a boolean. The Paste object will be used for the clipboard app.

>
> """
> #include <com/ubuntu/content/item.h>
> """
>
> What do you use from this header?

That can be removed

>
> """
> namespace com
> {
> namespace ubuntu
> {
> namespace content
> {
> namespace detail
> {
> }
> }
> }
> }
> """
>
> You can remove this as you don't forward-declare anything in there.
>
> """
> Q_PROPERTY(int id READ id)
> [...]
> Q_PROPERTY(QString source READ source)
> """
>
> If those properties never change you should declare them as CONSTANT, like
> this:
> Q_PROPERTY(QString source READ source CONSTANT)

Agreed, that would be better

>
> """
> enum State
> {
> created,
> charged,
> saved,
> collected
> };
> """
>
> What do those states mean and why should user the care about them all?

The user probably doesn't care, and we might just blow away the state property.

>
>
> """
> Q_INVOKABLE virtual int id() const;
> Q_INVOKABLE virtual State state() const;
> Q_INVOKABLE virtual bool charge(const QMimeData& mimeData);
> Q_INVOKABLE virtual QMimeData* mimeData();
> Q_INVOKABLE virtual QString source() const;
> """
>
> They don't have to (nor should) be Q_INVOKABLE if they're already getters or
> setters of Q_PROPERTIES. You added the Q_INVOKABLE so they could be called
> from QML, right?

Yes, although we probably only need the charge method to be callable from QML.

>
>
> """
> //Q_PROPERTY(QMimeData mimeData READ mimeData WRITE charge)
> """
>
> Why commented out?

I don't see this commented out, maybe you reviewed this before my last push.

>
> """
> Q_INVOKABLE virtual bool charge(const QMimeData& mimeData);
> """
>
> Is the user supposed to call this? What's the use case? He can call
> Hub::create_paste(mimeData) and later change the paste contents passing a
> different QMimeData?

Not directly, this is for use by the hub. Once the Paste object is charged, it can no longer be charged again. So the data won't change. For qtubuntu, you should only use the API exposed by hub.h.

>
> What's the use of Paste::source property?

This is the appId of the application that created the paste. We'll use this in the clipboard app to show the application icon next to the item in the stack.