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 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<http://techbase.kde.org/Development/Architecture/KDE4/XMLGUI > > _Technology>, so I don't see any reason > > to avoid it and its also the recommended one. > > 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 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<http://techbase.kde.org/Development/Architecture/KDE4/XMLGUI_Technology>, so I don't see any reason to avoid it and its also the recommended one. P.S.: [1] As it regards issues like those, we can always disagree about stuff like that but the point is to focus on the major issues. -- Giorgos Tsiapaliokas (terietor) KDE Developer terietor.gr
Re: KDEREVIEW: share like connect and plasmate
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
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
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
