Re: KDEREVIEW: share like connect and plasmate

[Ben Cooksley]

On Thu, Jan 31, 2013 at 12:47 PM, Aaron J. Seigo  wrote:
> On Thursday, January 31, 2013 10:43:29 Ben Cooksley wrote:
>> > as it has been mentioned plasmate is ready to go into the extragear.
>> > Can you move it to the extragear?
>>
>> Where precisely in Extragear is Plasmate
>
> SDK
>
>> and Share-Like-Connect headed?
>
> Base
>
> thanks :)

Both Plasmate and Share-Like-Connect have now been moved to the
appropriate locations.

>
> --
> Aaron J. Seigo

Regards,
Ben Cooksley
KDE Sysadmin

Re: KDEREVIEW: share like connect and plasmate

[Aaron J. Seigo]

On Thursday, January 31, 2013 10:43:29 Ben Cooksley wrote:
> > as it has been mentioned plasmate is ready to go into the extragear.
> > Can you move it to the extragear?
> 
> Where precisely in Extragear is Plasmate

SDK

> and Share-Like-Connect headed?

Base

thanks :)

-- 
Aaron J. Seigo

signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Ben Cooksley]

On Wed, Jan 30, 2013 at 11:23 PM, Giorgos Tsiapaliokas
 wrote:
> Hello,

Hi Giorgos,

>
> as it has been mentioned plasmate is ready to go into the extragear.
> Can you move it to the extragear?

Where precisely in Extragear is Plasmate and Share-Like-Connect headed?
They are both overdue to be moved from KDE Review.

Note: Moves into and out of KDE Review must *always* be CC'ed to kde-core-devel.

On that note: any last objections?

>
> Regards,
> Giorgos

Regards,
Ben Cooksley
KDE Sysadmin

>
>
> --
> Giorgos Tsiapaliokas (terietor)
> KDE Developer
>
> terietor.gr
>
> ___
> Plasma-devel mailing list
> plasma-de...@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>

Re: KDEREVIEW: share like connect and plasmate

[Aaron J. Seigo]

On Monday, January 7, 2013 22:58:12 Ben Cooksley wrote:
> On Sun, Jan 6, 2013 at 2:10 PM, Aaron J. Seigo  wrote:
> > On Thursday, January 3, 2013 09:56:47 Ben Cooksley wrote:
> >> What about Share-Like-Connect?
> > 
> > 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.
> 
> Where is it destined? This was never stated it seems.

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.

-- 
Aaron J. Seigo

signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Ben Cooksley]

On Sun, Jan 6, 2013 at 2:10 PM, Aaron J. Seigo  wrote:
> On Thursday, January 3, 2013 09:56:47 Ben Cooksley wrote:
>> What about Share-Like-Connect?
>
> 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.

Where is it destined? This was never stated it seems.

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

Regards,
Ben

>
> --
> Aaron J. Seigo
> ___
> Plasma-devel mailing list
> plasma-de...@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>

Re: KDEREVIEW: share like connect and plasmate

[Aaron J. Seigo]

On Thursday, January 3, 2013 09:56:47 Ben Cooksley wrote:
> What about Share-Like-Connect?

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

-- 
Aaron J. Seigo

signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Aaron J. Seigo]

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:
> Alle giovedì 3 gennaio 2013, Giorgos Tsiapaliokas ha scritto:
> > some comments in this review aren't productive and this makes the
> > whole process harder..
>
> 
> 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.
> 

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.

--
Aaron J. Seigo

signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Pino Toscano]

Alle giovedì 3 gennaio 2013, Giorgos Tsiapaliokas ha scritto:
> On 3 January 2013 01:51, Pino Toscano  wrote:
> > - PasswordAsker sounds like could be implemented on top of
> > KPasswordDialog
> 
> 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.

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

> QXmlStreamWriter::writeNamespace could be a guess.
> 
> 
> plasmate is using QXmlStreamWriter::writeDefaultNamespace, so which
> is the issue?

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.

> > > this is the only documented solution in
> > > techbase > > LGUI _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.
> 
> again, what's the point of doing this?

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.

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


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.


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

Can you please provide links to them?

-- 
Pino Toscano


signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Giorgos Tsiapaliokas]

On 3 January 2013 01:51, Pino Toscano  wrote:
> - PasswordAsker sounds like could be implemented on top of
> KPasswordDialog

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?



> > this is the only documented solution in
> > techbase > _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.


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.

-- 
Giorgos Tsiapaliokas (terietor)
KDE Developer

terietor.gr

Re: 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  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  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 _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


signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Giorgos Tsiapaliokas]

On 2 January 2013 23:38, Pino Toscano  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.


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

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

> - why ImageLoader::run forces the formats?
>
> Still there.


yes we missed that


> > - why KConfigXtWriter writes  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?


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

-- 
Giorgos Tsiapaliokas (terietor)
KDE Developer

terietor.gr

Re: KDEREVIEW: share like connect and plasmate

[Ben Cooksley]

On Thu, Jan 3, 2013 at 10:38 AM, Pino Toscano  wrote:
> 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:
>> - PasswordAsker sounds like could be implemented on top of
>> KPasswordDialog
>
> Still there.
>
>> - 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.
>
>> - 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.
>
>> - 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
>
> Still there, and it is even worse now: KStandardDirs is used to get a
> path for a _remote_ location.
>
>> - 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)
>
> Still there; the only change here was just using KLocale for the date
> output.
>
>> -  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
>
> Still there.
>
>> - EditPage::showTreeContextMenu uses the internalPointer() of the
>> model, which makes it prone to break if the model changes
>> implementation internally
>
> Still there.
>
>> - why ImageLoader::run forces the formats?
>
> Still there.
>
>> - why KConfigXtWriter writes  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...
>
>> - 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?

Due to this review issue, Plasmate has been returned to KDE Review. My
query regarding Share-Like-Connect still stands as well.

>
> --
> Pino Toscano
>

Regards,
Ben

> ___
> Plasma-devel mailing list
> plasma-de...@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>

Re: KDEREVIEW: share like connect and plasmate

[Pino Toscano]

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:
> - PasswordAsker sounds like could be implemented on top of
> KPasswordDialog

Still there.

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

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

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

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

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

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

Still there.

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

Still there.

> - why ImageLoader::run forces the formats?

Still there.

> - why KConfigXtWriter writes  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...

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

-- 
Pino Toscano


signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Ben Cooksley]

On Thu, Jan 3, 2013 at 7:56 AM, Sebastian Kügler  wrote:
> On Tuesday, January 01, 2013 15:39:27 Ben Cooksley wrote:
>> On Tue, Jan 1, 2013 at 3:31 AM, Antonis Tsiapaliokas 
> wrote:
>> > Hello,
>> >
>> > We would like to inform you that all the above issues of the plasmate has
>> > been fixed.
>> > Can someone move it to extragear?
>>
>> Which project(s) does this concern?
>> Also, does anyone have any final reviews to make before I perform the move?
>
> 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.

Plasmate has now been moved to Extragear/SDK.
What about Share-Like-Connect?

> --
> sebas
>
> http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9

Regards,
Ben

Re: KDEREVIEW: share like connect and plasmate

[Sebastian Kügler]

On Tuesday, January 01, 2013 15:39:27 Ben Cooksley wrote:
> On Tue, Jan 1, 2013 at 3:31 AM, Antonis Tsiapaliokas  
wrote:
> > Hello,
> > 
> > We would like to inform you that all the above issues of the plasmate has
> > been fixed.
> > Can someone move it to extragear?
> 
> Which project(s) does this concern?
> Also, does anyone have any final reviews to make before I perform the move?

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

http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9

Re: KDEREVIEW: share like connect and plasmate

[Giorgos Tsiapaliokas]

On 1 January 2013 04:39, Ben Cooksley  wrote:
> On Tue, Jan 1, 2013 at 3:31 AM, Antonis Tsiapaliokas  wrote:
>> Hello,
>>
>> We would like to inform you that all the above issues of the plasmate has
>> been fixed.
>> Can someone move it to extragear?
>
> Which project(s) does this concern?

It's just about plasmate.

Regards,
Giorgos

-- 
Giorgos Tsiapaliokas (terietor)
KDE Developer

terietor.gr

Re: KDEREVIEW: share like connect and plasmate

[Antonis Tsiapaliokas]

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

[Ben Cooksley]

On Tue, Jan 1, 2013 at 3:31 AM, Antonis Tsiapaliokas  wrote:
> Hello,
>
> We would like to inform you that all the above issues of the plasmate has
> been fixed.
> Can someone move it to extragear?

Which project(s) does this concern?
Also, does anyone have any final reviews to make before I perform the move?

>
> Cheers,
> Antonis

Thanks,
Ben Cooksley
KDE Sysadmin

>
> ___
> Plasma-devel mailing list
> plasma-de...@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>

Re: KDEREVIEW: share like connect and plasmate

[Antonis Tsiapaliokas]

>
> Did you actually understand what I'm referring to?
>

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

[Aaron J. Seigo]

On Thursday, November 8, 2012 20:06:10 Antonis Tsiapaliokas wrote:
> > - PasswordAsker sounds like could be implemented on top of
> > KPasswordDialog
> 
> gpgme is using pinetry-qt4 for password prompt, i don't think that we
> should use the KPasswordDialog.

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.

-- 
Aaron J. Seigo

signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Pino Toscano]

Alle giovedì 8 novembre 2012, Antonis Tsiapaliokas ha scritto:
> > - PasswordAsker sounds like could be implemented on top of
> > KPasswordDialog
> 
> gpgme is using pinetry-qt4 for password prompt, i don't think that we
> should use the KPasswordDialog.

Did you actually understand what I'm referring to?

-- 
Pino Toscano


signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Antonis Tsiapaliokas]

> - PasswordAsker sounds like could be implemented on top of
> KPasswordDialog
>

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

KDEREVIEW: share like connect and plasmate

[David Edmundson]

Initial code review of Share-Like-Connect:

from provider.h

~Provider();

This class is designed to be subclassed, yet the destructor is public
and non virtual. The derived classes will leak everything.

--

virtual QVariant executeAction(SLC::Provider::Action action, const
QVariantHash &content, const QVariantHash ¶meters

This implies the execute action is synchronous. Given this is meant to
(as I understand it) include uploading to the web in later releases
this will cause all of plasma-desktop to lock up whilst the upload
takes place. Again why is it a QVariant? Everything returns a bool.

In the case of the identica plugin, you start calling a webpage then
return true immediately... but then who is responsible for showing if
opening the website failed? Currently it will just fail silently and
the user will get false positive feedback?

Returning a KJob* would fix both of the above.

--

virtual Actions actionsFor(const QVariantHash &content) const;

Completely undocumented QVariant maps do not make for a good API. A
good API should be usable without the documentation, this is not. To
me this looks like poor code to pander to QML's limitation. At the
very least it needs documentation.

--
virtual QString actionName(const QVariantHash &content, Action action);

This can be const?


--

in
RatingProvider::executeAction

int rating = parameters["Targets"].toStringList().first().toInt();

calling .first() on a stringlist is liable to crash if the stringlist is empty.

(in fairness, there's a FIXME next to it too)

--

rating/tags

you should probably check nepomuk is enabled in the actionsFor()
method. Especially as you always return true regardless of whether
setTags failed or not


--

RatingProvider::exectuteAction

if (content.value("Mime Type").toString() == "text/x-html")

should be
== QLatin1String("text/x-html");

(this is in many places)

--

David Edmundson

Re: KDEREVIEW: share like connect and plasmate

[Albert Astals Cid]

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

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

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

[Pino Toscano]

Alle lunedì 5 novembre 2012, Giorgos Tsiapaliokas ha scritto:
> On 4 November 2012 16:55, Pino Toscano  wrote:
> > - the following binaries are installed in $prefix/bin:
> >   - plasmaengineexplorer
> >   - plasmakconfigxteditor
> >   - plasmaremoteinstaller
> >   - plasmate
> >   - plasmawallpaperviewer
> >   - plasmoidviewer
> >   - remote-widgets-browser (*)
> >   - windowswitcherpreviewer (*)
> > 
> > except the plasma-something ones (including the notable exception
> > of plasmoidviewer), the two I marked with (*) look too generic to
> > be installed in bindir; please consider moving them to libexecdir
> > (making sure to use KStandardDirs::findExe to reach them), or give
> > them less generic names
> 
> I would prefer to change their names. What do you think?

You are the developer of plasmate, not me, so pick any of the solution I 
expressed above or go with your own, if you have a better one.

-- 
Pino Toscano


signature.asc
Description: This is a digitally signed message part.

Re: 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  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  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
> 
> 
> 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"; >
>   
> 
>   
>.
>   
> 
> 
> 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 f

Re: KDEREVIEW: share like connect and plasmate

[Giorgos Tsiapaliokas]

On 4 November 2012 16:55, Pino Toscano  wrote:
> - the following binaries are installed in $prefix/bin:
>   - plasmaengineexplorer
>   - plasmakconfigxteditor
>   - plasmaremoteinstaller
>   - plasmate
>   - plasmawallpaperviewer
>   - plasmoidviewer
>   - remote-widgets-browser (*)
>   - windowswitcherpreviewer (*)
> except the plasma-something ones (including the notable exception of
> plasmoidviewer), the two I marked with (*) look too generic to be
> installed in bindir; please consider moving them to libexecdir (making
> sure to use KStandardDirs::findExe to reach them), or give them less
> generic names

I would prefer to change their names. What do you think?

Regards,
Giorgos

P.S.: sorry for my late reply on the topic


-- 
Giorgos Tsiapaliokas (terietor)
KDE Developer

terietor.gr

Re: KDEREVIEW: share like connect and plasmate

[Giorgos Tsiapaliokas]

Hello,


On 3 November 2012 19:35, Pino Toscano  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  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


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"; >
  

  
   .
  


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

Re: KDEREVIEW: share like connect and plasmate

[Aaron J. Seigo]

On Sunday, November 4, 2012 15:55:00 Pino Toscano wrote:
> > Regarding plasmate: I fixed earlier a number of i18n issues (ugh...),
> > and the layout of two .ui files.
> 
> (A couple more today...)

Thanks for your thorough review; I'll make sure the people working on Plasmate 
start processing your findings and implemening fixes for them :)

-- 
Aaron J. Seigo

signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Pino Toscano]

Hi,

Alle sabato 3 novembre 2012, Pino Toscano ha scritto:
> Alle giovedì 1 novembre 2012, Aaron J. Seigo ha scritto:
> > 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.
> 
> Regarding plasmate: I fixed earlier a number of i18n issues (ugh...),
> and the layout of two .ui files.

(A couple more today...)

> On the other hand, there are still the following issues I found
> looking around:

- in plasmate/publisher/remoteinstaller/remoteinstaller.ui there is a 
label: «Choose the source directory of your package.\n\nYour URL must 
end in a metadata.desktop file.», but the URL selector below (named 
srcDirUrl) wants a file and its value is used only as directory (see 
remoteinstaller.cpp)

- there is a bit of string mess for some terms:
  - "Add-On" vs "Addon", used with the capital letter even in middle of
sentences
  - "Save Point" vs "SavePoint" (both upper- and lower- case)
I'd recommend (after fixing the .ui files and other issues, as already 
noted in my other email) taking a deep review of the strings of plasmate 
(see plasmate.pot in svn [1]) according to the few bits of HIG [2] we 
have.

- plasmate uses the "plasmagick" icon, which exists in the Oxygen icon 
theme only; this means it will have no icon when the icon theme is 
another one, or simply in menus of other DEs; please copy it from Oxygen 
(or get a non-Oxygen generic one) and install it as hicolor.

- the following binaries are installed in $prefix/bin:
  - plasmaengineexplorer
  - plasmakconfigxteditor
  - plasmaremoteinstaller
  - plasmate
  - plasmawallpaperviewer
  - plasmoidviewer
  - remote-widgets-browser (*)
  - windowswitcherpreviewer (*)
except the plasma-something ones (including the notable exception of 
plasmoidviewer), the two I marked with (*) look too generic to be 
installed in bindir; please consider moving them to libexecdir (making 
sure to use KStandardDirs::findExe to reach them), or give them less 
generic names

[1] trunk/l10n-kde4/templates/messages/kdereview/plasmate.pot
[2] http://techbase.kde.org/Projects/Usability/HIG

Thanks,
-- 
Pino Toscano


signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Kevin Krammer]

On Saturday, 2012-11-03, Pino Toscano wrote:

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

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

Cheers,
Kevin
-- 
Kevin Krammer, KDE developer, xdg-utils developer
KDE user support, developer mentoring


signature.asc
Description: This is a digitally signed message part.

Re: KDEREVIEW: share like connect and plasmate

[Pino Toscano]

Hi,

Alle giovedì 1 novembre 2012, Aaron J. Seigo ha scritto:
> 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.

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

-- 
Pino Toscano


signature.asc
Description: This is a digitally signed message part.

KDEREVIEW: share like connect and plasmate

[Aaron J. Seigo]

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.

-- 
Aaron J. Seigo

signature.asc
Description: This is a digitally signed message part.