Code review comment for lp://qastaging/~verzegnassi-stefano/ubuntu-docviewer-app/document-hub2

Revision history for this message
Riccardo Padovani (rpadovani) wrote :

> Sorry if I reply to the inline comments here, but it's for my brain sanity
> when I will fix them. :-)

I completely understand :D

> >Line 3783: not sure which line do you mean?
> >> If (import "../upstreamComponents”)
> The EmptyState item lives in ../upstreamComponents

Oh, right, my fault here!

> > Line 4510:
> Nope! As in gallery-app, the button is not enabled but still visible, to
> advice that the content transfer could not be done if no item has been
> selected.
> IMHO it should be shown in any case.

You have a point here, I agree with you

> >src/app/qml/textView/TextView.qml
> >Why do you use a TextArea with readOnly: true and not a Label?
> Copy and paste! Even if the TextArea is read-only, they are still useful.

Aha, right! Didn't think about it, sorry

> > src/app/quick/documentMode.cpp
> > Could you please explain what are you trying to achieve with DocumentModel:
> > Seems improvable, but I'm not sure I understand how you want to use it
> When there's a change in the directory we're watching (e.g. document added or
> removed), we want to update the model according to the changes:
> The code in that slot just removes all the entries from that directory and re-
> parse its content.
> The two ints, n and m, are used to keep a note of the number of removed items,
> so that we can exactly remove the rows from the model (by doing so we avoid to
> have duplicated undefined rows).

Oh, I see, makes sense.
I think could be improvable, but atm works well, so it's ok

> About the ENUM, I will do in a next MP. At the moment it works, but I have no
> resources (most of all, time) for staying in-sync with the tasks I still need
> to do for the docviewer.
> The same observation is also valid for the issues you reported that are
> related to the gallery-app.

I fully understand that, there are simply too much work to do :-)

> For all the rest I will provide a fix hopefully today (max. tomorrow).

Ok, I'll do another quick review after then I'll approve it, thanks!

> Thank you Riccardo for the huge review.

Thanks to you for the amazing work!

« Back to merge proposal