DevHeads.net

KDEREVIEW: share like connect and plasmate

Hello all ...

This is to inform everyone that the plasmate and share-like-connect
repositories have been moved into KDE Review so that, if all goes according to
plan, we can move them to their more permanent homes in a couple of weeks.

Most of the apps in the plasmate repo have actually been in past SC releases;
it's just the main app, plasmate, that is still fairly new. It's on the road
to a 1.0 release, however, and now is a convenient time to get it re-homed.

Share Like Connect is a bit of Plasma powered UI that lets one does what it
says: share, like and connect whatever it is you are looking at (assuming that
what you are looking at it with uses ResourceInstance to let us know :). We've
shipped it in three Plasma Active releases and have made it spiffy for the
desktop so we may include it in the layout in future.

Comments

Re: KDEREVIEW: share like connect and plasmate

By Albert Astals Cid at 11/05/2012 - 16:57

El Dijous, 1 de novembre de 2012, a les 20:06:36, Aaron J. Seigo va escriure:
Hi

share-like-connect applet folder has i18n calls but no Messages.sh. Same thing
for the sendbyemail provider.

Also, you can only share by email right now? Seems a bit limited to me, but i
guess that's better than nothing, at least it'll calm down the people asking
me to add a "send by email" menuitem to Okular.

Cheers,
Albert

Re: KDEREVIEW: share like connect and plasmate

By Pino Toscano at 11/03/2012 - 13:35

Hi,

Alle giovedì 1 novembre 2012, Aaron J. Seigo ha scritto:
Regarding plasmate: I fixed earlier a number of i18n issues (ugh...),
and the layout of two .ui files.
On the other hand, there are still the following issues I found looking
around:

- there are some dialogs being done as QDialog: what about converting
them to KDialog, for a common look'n'feel with the rest of KDE apps, and
a bit less layouting code?

- PasswordAsker sounds like could be implemented on top of
KPasswordDialog

- BranchDialog sounds like could be replaced with KInputDialog::getText
with a custom validator

- CommitDialog, other than being a KDialog, should better be use layouts
instead of placing widgets manually

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

- metadata.ui has an error message whose color is forced as red; please
use KMessageWidget or at least use KColorScheme to get the fore-/back-
ground colors for errors

- 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); also there is a race between the KIO
exists and the mkdir calls

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

- 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? also, such handler currently writes to
/var/tmp/plasmatepreviewerlog.txt, which is not a good thing to do...

- 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

- KonsolePreviewer (ab)uses /var/tmp/plasmatepreviewerlog.txt as
temporary file name, please use KTemporaryFile

- wrt Publisher::doCMake:
- should check that `cmake` exists (see KStandardDirs::findExe) prior
to do all the work
- $KDEDIRS is not set by default by KDE, but only by the user
- check the return value of a command invocation, before starting the
next command
- when commands fail, instead of a generic failure error, what about
showing the output+error log?

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

- why ImageLoader::run forces the formats?

- 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

- why KConfigXtWriter writes <kcfg> prologue/epilogue by hand?

- TextEditor::modifyToolBar does a big no-no job in looking for actions
(never ever compare to translated strings, especially when coming from
other components)

I think that's enough for now.

Re: KDEREVIEW: share like connect and plasmate

By Pino Toscano at 01/02/2013 - 17:38

Hi,

apparently some people consider that all the issues of this review have
been fixed, but really they were not.

Alle sabato 3 novembre 2012, Pino Toscano ha scritto:
Still there.

Still there.

Still there.

Still there.

Still there, and it is even worse now: KStandardDirs is used to get a
path for a _remote_ location.

Still there; the only change here was just using KLocale for the date
output.

Still there.

Still there.

Still there.

Now it writes the namespaces in a wrong way, closing the quoting
manually and adding attributes by hand in a single string...

What about just finding the actions in the actionCollection() of the
KTextEditor::View, and hiding them, instead of messing up with the
XMLGUI document?

Re: KDEREVIEW: share like connect and plasmate

By Tsiapaliwkas Giorgos at 01/02/2013 - 19:09

On 2 January 2013 23:38, Pino Toscano < ... at kde dot org> wrote:
*[1] The 2 above are some good impovements for the future, but not something
that should keep plasmate in playground.

In which ui files are you referring to?

*[1] the above 3 are good improvements but not something which should
keep plasmate in playground.

yes we missed that

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?

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.

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.

Re: KDEREVIEW: share like connect and plasmate

By Pino Toscano at 01/02/2013 - 19:51

Alle giovedì 3 gennaio 2013, Giorgos Tsiapaliokas ha scritto:
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*.

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

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

QXmlStreamWriter::writeNamespace could be a guess.

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

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.

Re: KDEREVIEW: share like connect and plasmate

By Tsiapaliwkas Giorgos at 01/03/2013 - 12:12

On 3 January 2013 01:51, Pino Toscano < ... at kde dot org> wrote:
we have asked in #kde-devel and we have been informed that kdialogpassword
isn't a safe replacement for pinetry, so this isn't an issue.

QXmlStreamWriter::writeNamespace could be a guess.

plasmate is using QXmlStreamWriter::writeDefaultNamespace, so which is the
issue?

again, what's the point of doing this?

some comments in this review aren't productive and this makes the whole
process
harder..

Regards,
Giorgos

P.S.: I have just opened some reviews regarding the issues.

Re: KDEREVIEW: share like connect and plasmate

By Pino Toscano at 01/03/2013 - 12:37

Alle giovedì 3 gennaio 2013, Giorgos Tsiapaliokas ha scritto:
You still are not getting the issue, at all.
What I'm referring to is the PasswordAsker class in
plasmate/publisher/signingwidget.cpp, derived from QDialog and
GpgME::PassphraseProvider, which is being created (line 192) and set as
passphrase provider (line 194). The cases (and so the solutions) are
two:
a) that passphrase provider is used: then you fix PasswordAsker to be
based on KPasswordDialog
b) that passphrase provider is not used at all: then you just drop the
code

The issue is that you are not writing "a default namespace", but hacking
to write also other attributes for the root element; this is basically
similar of doing something like:
writer.writeAttribute("foo", "bar\" foo2=\"bar2\"")
which is wrong, since you're bypassing the way the various XML data is
composed together to build a document.

Use a simplier way (get the action from the actionCollection(), and
setVisible(false) it) instead of an hackish way to manipulate the XMLGUI
document, which could break less easily.

<rant>
Right, it would have been easier if you would had started *reading* and
trying to *understand* all my comments from day #1, instead of dismiss
half of them and underestimate the other half.
</rant>

Can you please provide links to them?

Re: KDEREVIEW: share like connect and plasmate

By Aaron J. Seigo at 01/05/2013 - 21:08

hi ..

feedback was taken into consideration; fixes were made; some issues have been
punted to the next release so we can practice "release early, release often,
make it better each release" rather than "release when it is perfect, namely
never".

please move plasmate out of kdereview so we can get back to making a release
and starting on the next iteration of improvements.

a more detailed response follows...

On Thursday, January 3, 2013 17:37:21 Pino Toscano wrote:
i hate coming back from a few days off to find things like this in my email.
it's so unecessarily discouraging.

i'm very happy with the review feedback we received and it was all greatly
appreciated. the developers involved have worked hard to incorporate
improvements based on them. i know because i spent quite a bit of time
reviewing some of those changes.

however:

* being defect free is not a prerequesite for being moved out of kdereview. i
think we are getting confused between "ready to move out of kdereview" with
"perfect". plasmate works, it is translatable, it follows other basic
policies, etc. the developers working on it are really itching to get a first
release out the door, and having it stuck in kde-review until it is deemed
perfect is not helping that occur.

* insisting on things like a dialog must be based on KPasswordDialog prior to
the whole app making its way out of kdereview is utterly absurd. particularly
when in this case, it doesn't actually matter. i don't think that dialog is
even used ... so yes, that code can be culled ... leaving it there doesn't
hurt anything, however, and insisting on non-problem "issues" being attended
to when the developers involved are focused on actual-problem issues is not
helpful.

* some valid issues raised during kdereview feedback have been scheduled for
future releases; other issues were deemed invalid after further discussion. i
don't like someone who is not involved either with the code in question or the
release management of the project being able to override either of those sets
of decisions. if those decisions were in violation of some kde policy (e.g.
"must be translatable"), fine; but they aren't in that category of issues.
instead, it's mostly "this could be better" or "i don't like how that
particular block of code has been written".

thanks.

Re: KDEREVIEW: share like connect and plasmate

By Ben Cooksley at 01/02/2013 - 17:41

On Thu, Jan 3, 2013 at 10:38 AM, Pino Toscano < ... at kde dot org> wrote:
Due to this review issue, Plasmate has been returned to KDE Review. My
query regarding Share-Like-Connect still stands as well.

Regards,
Ben

Re: KDEREVIEW: share like connect and plasmate

By Antonis Tsiapaliokas at 11/08/2012 - 14:06

gpgme is using pinetry-qt4 for password prompt, i don't think that we
should use the KPasswordDialog.

Re: KDEREVIEW: share like connect and plasmate

By Aaron J. Seigo at 11/08/2012 - 16:53

On Thursday, November 8, 2012 20:06:10 Antonis Tsiapaliokas wrote:
gpgme does this because pinentry-qt4 is integrated with the (needs to be
secure) gpg passphrase infrastructure.

that is not the case here, so it can easily use KPasswordDialog.

Re: KDEREVIEW: share like connect and plasmate

By Pino Toscano at 11/08/2012 - 16:51

Alle giovedì 8 novembre 2012, Antonis Tsiapaliokas ha scritto:
Did you actually understand what I'm referring to?

Re: KDEREVIEW: share like connect and plasmate

By Antonis Tsiapaliokas at 11/08/2012 - 19:42

I guess that we all agree that we should replace the QDialog with the
KPasswordDialog (right?).
If so, how can we do that? Even if someone doesn't have the pinentry-qt4,
then the pinentry (CL)
is opening. And i wasn't able to remove the pinentry. (Pinentry is being
required by the gpg2).

Right now, plasmate doesn't use the "QDialog". (For example if your try to
remove some of the
UI widgets, then the UI doesn't change...)

So how can i make the gpgme to use the QDialog/KPasswordDialog instead of
the pinentry-qt4?

Re: KDEREVIEW: share like connect and plasmate

By Antonis Tsiapaliokas at 12/31/2012 - 10:31

Hello,

We would like to inform you that all the above issues of the plasmate has
been fixed.
Can someone move it to extragear?

Cheers,
Antonis

Re: KDEREVIEW: share like connect and plasmate

By Ben Cooksley at 12/31/2012 - 22:39

On Tue, Jan 1, 2013 at 3:31 AM, Antonis Tsiapaliokas < ... at gmail dot com> wrote:
Which project(s) does this concern?
Also, does anyone have any final reviews to make before I perform the move?

Thanks,
Ben Cooksley
KDE Sysadmin

Re: KDEREVIEW: share like connect and plasmate

By Sebastian =?utf... at 01/02/2013 - 14:56

On Tuesday, January 01, 2013 15:39:27 Ben Cooksley wrote:
We've been reviewing all individual patches on the plasma list. If someone
wants to have another go, feel free, but I consider it good quality.

Re: KDEREVIEW: share like connect and plasmate

By Ben Cooksley at 01/02/2013 - 16:56

On Thu, Jan 3, 2013 at 7:56 AM, Sebastian Kügler < ... at kde dot org> wrote:
Plasmate has now been moved to Extragear/SDK.
What about Share-Like-Connect?

Regards,
Ben

Re: KDEREVIEW: share like connect and plasmate

By Aaron J. Seigo at 01/05/2013 - 21:10

On Thursday, January 3, 2013 09:56:47 Ben Cooksley wrote:
i was waiting until i was back in the office with time to work on it again
before requesting the move. :)

so ... yes, SLC is ready to be moved out of kdereview.

we have it working properly on desktop as well as for touch devices and while
not all of our (internal) API modifications are where we want them to be, it is
sufficiently developed for another proper release ... (we've already made 3
releases of it with Plasma Active, fww)

thanks in advance :)

Re: KDEREVIEW: share like connect and plasmate

By Ben Cooksley at 01/07/2013 - 05:58

On Sun, Jan 6, 2013 at 2:10 PM, Aaron J. Seigo < ... at kde dot org> wrote:
Where is it destined? This was never stated it seems.

Regards,
Ben

Re: KDEREVIEW: share like connect and plasmate

By Aaron J. Seigo at 01/07/2013 - 06:19

On Monday, January 7, 2013 22:58:12 Ben Cooksley wrote:
good question :) i suppose extragear/base.

in future, i want to use it as part of the default layout of the panel if it
is installed, but that will be at least one more release and we may even
postpone that until a libplasma2 based shell hapens. but eventually there will
be a soft run-time dep on from kde-workspace on this repository ... but this
is not something that prevents it going into extragear.

the "many repository" approach is going to make releases of the desktop
workspaces more and more ... "interesting" :) we may end up needing to adjust
how we define such releases at some point in the future. the Frameworks 5 era
seems like a good time to examine if changes would be beneficial for workspace
releases.

Re: KDEREVIEW: share like connect and plasmate

By Tsiapaliwkas Giorgos at 01/01/2013 - 14:51

On 1 January 2013 04:39, Ben Cooksley < ... at kde dot org> wrote:
It's just about plasmate.

Regards,
Giorgos

Re: KDEREVIEW: share like connect and plasmate

By Tsiapaliwkas Giorgos at 11/05/2012 - 10:26

Hello,

On 3 November 2012 19:35, Pino Toscano < ... at kde dot org> wrote:
and which is the correct way in order to fix this?

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

what do you mean?

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.

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.

what should it use?

where is this code?

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

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)

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.

what do you mean?

Re: KDEREVIEW: share like connect and plasmate

By Pino Toscano at 11/05/2012 - 11:06

Hi,

Alle lunedì 5 novembre 2012, Giorgos Tsiapaliokas ha scritto:
Just set a bold font at runtime?

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

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

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.

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.

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

Query the data() of the model.

KPIMUtils::EmailValidator, part of kdepimlibs

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

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

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

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

Re: KDEREVIEW: share like connect and plasmate

By Kevin Krammer at 11/03/2012 - 14:41

I guess KStandardDirs could handle that, using resource type "tmp"

Cheers,
Kevin