KDEREVIEW: share like connect and plasmate

[Pino Toscano]

Alle giovedì 3 gennaio 2013, Giorgos Tsiapaliokas ha scritto:
> On 2 January 2013 23:38, Pino Toscano <pino at kde.org> wrote:
> > > - BranchDialog sounds like could be replaced with
> > > KInputDialog::getText with a custom validator
> > 
> > Still there.
> > 
> > > - CommitDialog, other than being a KDialog, should better be use
> > > layouts instead of placing widgets manually
> > 
> > Still there.
> 
> *[1] The 2 above are some good impovements for the future, but not
> something that should keep plasmate in playground.

Giving a fixed size to a dialog, and putting its widgets in a fixed 
place causes issues with:
a) different fonts than the ones the dialog was designed
b) texts of different size, like translated ones
so no, this is not an "improvement", this is a *bug*.

> > > - a numer of .ui files sets bold/bigger texts, but using a qt
> > > rich text which forces a font size (and in few cases also the
> > > font face)
> > 
> > Still there.
> 
> In which ui files are you referring to?

plasmate/publisher/publisher.ui 
plasmate/publisher/remoteinstaller/remoteinstaller.ui
plasmate/editors/kconfigxt/kconfigxteditor.ui
plasmate/startpage.ui

(Not that there are many .ui files in plasmate, so you could have even 
checked on your own.)

> > - TimeLine::loadTimeLine does a funky job in putting translated
> > bits
> > 
> > > among the git output; a better way would be parsing the output
> > > extracting the various details, and composing a new ad-hoc string
> > > (and the date would need localization, as the FIXME say)
> > > 
> > > 
> > > 
> > > -  StartPage::saveNewProjectPreferences saves the status of all
> > > the js/py/etc radio buttons separately... saving the index or
> > > the name of the active one would be much easier
> > > 
> > > 
> > > 
> > > - EditPage::showTreeContextMenu uses the internalPointer() of the
> > > model, which makes it prone to break if the model changes
> > > implementation internally
> > 
> > Still there.
> 
> *[1] the above 3 are good improvements but not something which should
> keep plasmate in playground.

They are issues which don't fit extragear either, and IMHO they are more 
close to playground-quality code.

> > > - why KConfigXtWriter writes <kcfg> prologue/epilogue by hand?
> > 
> > Now it writes the namespaces in a wrong way, closing the quoting
> > manually and adding attributes by hand in a single string...
> 
> When I was implementing this one I couldn't find some API in
> QXmlStreamWriter
> which does the job and I assume that the same applies for rest of the
> people who
> read my review, am I missing something?

QXmlStreamWriter::writeNamespace could be a guess.

> > - TextEditor::modifyToolBar does a big no-no job in looking for
> > 
> > > actions (never ever compare to translated strings, especially
> > > when coming from other components)
> > 
> > What about just finding the actions in the actionCollection() of
> > the KTextEditor::View, and hiding them, instead of messing up with
> > the XMLGUI document?
> 
> this is the only documented solution in
> techbase<http://techbase.kde.org/Development/Architecture/KDE4/XMLGUI
> _Technology>, so I don't see any reason
> to avoid it and its also the recommended one.

That's point #3, while point #2 is similar to what I suggested.

> P.S.: [1] As it regards issues like those,  we can always disagree
> about stuff like that
> but the point is to focus on the major issues.

There were many "major issues" back when I did my first reviews, and 
some of them still are present; you did and still are underestimating 
the importance/impact of the issues I reported.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20130103/3332f360/attachment.sig>