Re: Review board is partly broken?
Hello, a few minutes I come across to another issue. In one of my reviews I have created a new file which doesn't exist in the remote tree, a few days ago I was able to create the review request but now I can't update the review. I receive this error `The file server/lib/db/forum.js (revision d4dde01) was not found in the repository` Regards, Giorgos -- Giorgos Tsiapaliokas (terietor) terietor.org
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
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
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
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
