KDEREVIEW: share like connect and plasmate

[Pino Toscano]

Hi,

Alle lunedì 5 novembre 2012, Giorgos Tsiapaliokas ha scritto:
> On 3 November 2012 19:35, Pino Toscano <pino at kde.org> wrote:
> > - 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)
> 
> and which is the correct way in order to fix this?

Just set a bold font at runtime?

> >also there is a race between the KIO
> >
> > exists and the mkdir calls
> 
> what do you mean?

Checking whether a directory exist on a remote server and creating it 
are not atomic operations (and actually they aren't even a local 
machine), so between the check and the actual creation there can be 
something else creating it (leading to failures if the directory has not 
been created by you and you don't have the permissions to write in it).

> > - main installs a message handler which makes MainWindow emit a
> > signal... which is caught by itself: why not just put it in
> > main.cpp, and in case there is a main window notify it to write to
> > the konsole widget?
> 
> there is a qobject wrapper(MainWindowWrapper) which internally
> instantiates mainwindow(MainWindow).
> So in main the wrapper calls the method emitSendMessage.
> Q: why do we need the wrapper?
> A: if you open plasmate from a terminal and close it from the ui you
> will see a segmentation fault output.

So you are working around with an useless wrapper instead of finding out 
what's the real cause of the crash?
I've never seen a KDE application in KDE 3 and KDE 4 times requiring 
such a wrapper because "closing it from the ui produces a segmentation 
fault" (and if they did, they were application bugs).

> Q: why we emit a signal instead of calling customMessageHandler
> directly.
> A: a segfault will occur when we will close plasmate.

See above.

But still you have not got what I said earlier: instead of routing a 
debug message from the handler to the main window to a signal to the 
main window itself again, just output/log/save things in the handler 
directly.

> > also, such handler currently writes to
> > /var/tmp/plasmatepreviewerlog.txt, which is not a good thing to
> > do...
> > 
> > - KonsolePreviewer (ab)uses /var/tmp/plasmatepreviewerlog.txt as
> > temporary file name, please use KTemporaryFile
> 
> I guess you mean QTemporaryFile because the KTemporaryFile is
> deprecated.

KTemporaryFile is perfectly okay for KDE4, and since plasmate is a KDE4 
application, you *will* use it, since it respects KDE the path for 
temporary files.

> When I was implementing the feature I wanted to use
> QTemporaryFile but it deletes
> the file on destruction but we need the file in different scopes.

setAutoDelete(false)
(looking at the API docs isn't a bad idea, after all...)

> > - EditPage::showTreeContextMenu uses the internalPointer() of the
> > model, which makes it prone to break if the model changes
> > implementation internally
> 
> what should it use?

Query the data() of the model.

> > - SigningDialog::validateParams could use some already existing
> > email validation method (iirc there is a basic one in kdelibs and
> > a better one in kdepimlibs)
> 
> where is this code?

KPIMUtils::EmailValidator, part of kdepimlibs

> > - why ImageLoader::run forces the formats?
> 
> Do you mean s/QImage image(m_image.path(), "PNG JPG GIF
> JPEG");/QImage image(m_image.path());

That's the result, yes, but you haven't explained why it was done that 
way...

> > - KConfigXtEditor writes/replaces XML by hand... is that really a
> > good thing to do (think about proper escaping, etc)? consider just
> > using QDom/QXml for the job
> 
> KconfigXtEditor does 3 things:
> * reads a xml file(can be done with QtXml)
> * writes new stuff in a xml file(can be done with QtXml)
> * modifies a xml file(can't be done with QtXml)
> 
> > - why KConfigXtWriter writes <kcfg> prologue/epilogue by hand?

This is still unanswered: that class writes a new document using 
QXmlStreamWriter, but writes prologue/epilogue by hand.

> because we don't want to ruin the coding style,
> 
> this is taken from
> declarative-plasmoids/microblog/contents/config/main.xml
> 
> <?xml version="1.0" encoding="UTF-8"?>
> <kcfg xmlns="http://www.kde.org/standards/kcfg/1.0"
>       xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
>       xsi:schemaLocation="http://www.kde.org/standards/kcfg/1.0
>       http://www.kde.org/standards/kcfg/1.0/kcfg.xsd" >
>   <kcfgfile name=""/>
> 
>   <group name="General">
>                    .........
>   </group>
> </kcfg>
> 
> if we use QXmlStreamWriter every child's indentation will be
> increased and it will ruin the coding style.

Sorry, but this is nonsense, QXmlStreamWriter has methods for setting an 
indentation.
Beside this, you are missing the main point, which is that editing an 
XML file with search and replace can break the document, if it was not 
formatted/indented exactly in the way you expect it to. Furthermore, you 
then need to care yourself for properly XML-escaping stuff you write, 
which does not seem something you do...

> > - 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 do you mean?

| if (action->text() == "&Save" || action->text() == "Save &As...")
If an user has a translate kate, this code won't ever match.
If the strings are changed in kdelibs/kate, this code will badly break.
And no, adding i18n() to the above string is *wrong*, since if the 
translations of the kate strings and plasmate's is different this code 
won't ever match.
If you want to look for those two actions, either find other ways (ask 
to the kate people?) or drop this code.

-- 
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/kde-core-devel/attachments/20121105/29a14e95/attachment.sig>
-------------- next part --------------
_______________________________________________
Plasma-devel mailing list
Plasma-devel at kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel