----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105032/#review14379 -----------------------------------------------------------
Looks good to me (though I'm not too involved in plasma, so I won't click ship it). I have a several nitpicking issues below. Also if you're making a new app make sure to check with someone if you need a Messages.sh file. publisher/publisher.cpp <http://git.reviewboard.kde.org/r/105032/#comment11344> Be sure to add your name to the copyright notices when you make large change. Though in reality everyone forgets to do this :) publisher/remoteinstaller/remoteinstaller.h <http://git.reviewboard.kde.org/r/105032/#comment11345> Unless there's a good reason, use KUrl (and for the other url in this class) publisher/remoteinstaller/remoteinstaller.cpp <http://git.reviewboard.kde.org/r/105032/#comment11346> Is this display hack needed with the newly patched plasmapkg? publisher/remoteinstaller/remoteinstallerdialog.cpp <http://git.reviewboard.kde.org/r/105032/#comment11347> Normalise these. (i.e remove the consts and &'s) you don't need them when specifying the method in 'connect' http://marcmutz.wordpress.com/effective-qt/prefer-to-use-normalised-signalslot-signatures/ publisher/remoteinstaller/remoteinstallerdialog.cpp <http://git.reviewboard.kde.org/r/105032/#comment11349> This looks like it leaks. QScopedPointer perhaps? publisher/remoteinstaller/remoteinstallerdialog.cpp <http://git.reviewboard.kde.org/r/105032/#comment11350> Why is this needed? When you create m_installer, you pass this as the parent. publisher/remoteinstaller/remoteinstallerdialog.cpp <http://git.reviewboard.kde.org/r/105032/#comment11351> for single characters use ' ' rather than " ". That way Qt knows to search for a character (rather than a string which just happens to be one letter long) and uses a faster replace method. - David Edmundson On June 1, 2012, 6:11 p.m., Giorgos Tsiapaliwkas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105032/ > ----------------------------------------------------------- > > (Updated June 1, 2012, 6:11 p.m.) > > > Review request for Plasma. > > > Description > ------- > > Hello, > > in terietor/remoteinstaller I have create a remote installer. > The goal is to install the projects remotely to another computer. > > You access the remote install either inside plasmate or using the > plasmaremoteinstaller binary. > > > Diffs > ----- > > CMakeLists.txt 12f8a3a > publisher/publisher.h 5f40ae0 > publisher/publisher.cpp fd87364 > publisher/remoteinstaller/advancedmodewidget.ui PRE-CREATION > publisher/remoteinstaller/remoteinstaller.h PRE-CREATION > publisher/remoteinstaller/remoteinstaller.cpp PRE-CREATION > publisher/remoteinstaller/remoteinstaller.ui PRE-CREATION > publisher/remoteinstaller/remoteinstallerdialog.h PRE-CREATION > publisher/remoteinstaller/remoteinstallerdialog.cpp PRE-CREATION > publisher/remoteinstaller/standalone/main.cpp PRE-CREATION > publisher/remoteinstaller/standalone/plasmaremoteinstaller.h PRE-CREATION > publisher/remoteinstaller/standalone/plasmaremoteinstaller.cpp PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/105032/diff/ > > > Testing > ------- > > I couldn't find any bugs in the remote installer, but the proccess fails > during KIO::copy. > Apparently I am not writing correct something in the fish protocol. > Right now the remote installer tries to do the above > > KIO::copy("/home/terietor/.kde4/share/apps/plasmate/project, > "fish://user@ip/home/user") > > but it fails > > > Thanks, > > Giorgos Tsiapaliwkas > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
