KDEREVIEW: share like connect and plasmate

[Giorgos Tsiapaliokas]

Hello,


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?

> - RemoteInstaller uses "/var/tmp/plasmaremoteinstaller/" as destination
> directory, which is a bit too generic (at least appending the user name
> and chmod'ing it 600 would help);

Antonis is working in a patch which will replace the hard-coded path
with KStandarDirs.

>also there is a race between the KIO
> exists and the mkdir calls

what do you mean?

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

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

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


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

what should it use?

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

> - why ImageLoader::run forces the formats?

Do you mean s/QImage image(m_image.path(), "PNG JPG GIF JPEG");/QImage
image(m_image.path());


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

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.


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



-- 
Giorgos Tsiapaliokas (terietor)
KDE Developer

terietor.gr