Re: KDEREVIEW: share like connect and plasmate
On Thu, Jan 31, 2013 at 12:47 PM, Aaron J. Seigo ase...@kde.org 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
On Wed, Jan 30, 2013 at 11:23 PM, Giorgos Tsiapaliokas terie...@gmail.com 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
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
On Sun, Jan 6, 2013 at 2:10 PM, Aaron J. Seigo ase...@kde.org 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
On Monday, January 7, 2013 22:58:12 Ben Cooksley wrote: On Sun, Jan 6, 2013 at 2:10 PM, Aaron J. Seigo ase...@kde.org 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
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.. 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 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
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
On 3 January 2013 01:51, Pino Toscano p...@kde.org 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 techbasehttp://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. 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
Alle giovedì 3 gennaio 2013, Giorgos Tsiapaliokas ha scritto: On 3 January 2013 01:51, Pino Toscano p...@kde.org 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 techbasehttp://techbase.kde.org/Development/Architecture/KDE4/XM 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.. 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 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
On Tuesday, January 01, 2013 15:39:27 Ben Cooksley wrote: On Tue, Jan 1, 2013 at 3:31 AM, Antonis Tsiapaliokas kok...@gmail.com 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
On Thu, Jan 3, 2013 at 7:56 AM, Sebastian Kügler se...@kde.org wrote: On Tuesday, January 01, 2013 15:39:27 Ben Cooksley wrote: On Tue, Jan 1, 2013 at 3:31 AM, Antonis Tsiapaliokas kok...@gmail.com 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
On 2 January 2013 23:38, Pino Toscano p...@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. - 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 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? - 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 techbasehttp://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. -- Giorgos Tsiapaliokas (terietor) KDE Developer terietor.gr
Re: KDEREVIEW: share like connect and plasmate
Alle giovedì 3 gennaio 2013, Giorgos Tsiapaliokas ha scritto: On 2 January 2013 23:38, Pino Toscano p...@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 techbasehttp://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 signature.asc Description: This is a digitally signed message part.
Re: KDEREVIEW: share like connect and plasmate
On 1 January 2013 04:39, Ben Cooksley bcooks...@kde.org wrote: On Tue, Jan 1, 2013 at 3:31 AM, Antonis Tsiapaliokas kok...@gmail.com 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
On Tue, Jan 1, 2013 at 3:31 AM, Antonis Tsiapaliokas kok...@gmail.com 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
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
- 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.
Re: KDEREVIEW: share like connect and plasmate
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
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
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
Hello, On 3 November 2012 19:35, Pino Toscano p...@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
Re: KDEREVIEW: share like connect and plasmate
On 4 November 2012 16:55, Pino Toscano p...@kde.org 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
Hi, Alle lunedì 5 novembre 2012, Giorgos Tsiapaliokas ha scritto: On 3 November 2012 19:35, Pino Toscano p...@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
Re: KDEREVIEW: share like connect and plasmate
Alle lunedì 5 novembre 2012, Giorgos Tsiapaliokas ha scritto: On 4 November 2012 16:55, Pino Toscano p...@kde.org 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
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
KDEREVIEW: share like connect and plasmate
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 parameters 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
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
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
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 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. -- Pino Toscano signature.asc Description: This is a digitally signed message part.
Re: KDEREVIEW: share like connect and plasmate
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.
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. -- Aaron J. Seigo signature.asc Description: This is a digitally signed message part.
