> On Oct. 28, 2014, 9:32 a.m., David Faure wrote: > > src/application.cpp, line 785 > > <https://git.reviewboard.kde.org/r/120794/diff/2/?file=322403#file322403line785> > > > > It's a bit confusing because QCLP::process() is not the same, and also > > because of the other processCmdLine method here. Maybe call this one > > handleCmdLine()? Sorry for nitpicking, but if we are going to replace this > > in most other apps, might as well be perfect :-) > > > > parse = just the parsing > > process = parse and handle the builtin options (in QLCP) > > handle = handle the app options > > > > As far as the slot is concerned, it's ok to say that > > Application::processCmdLine is parse + handle the app options, it's > > "builtin to Application". So I would rename this method, but leave the > > other one (processCmdLine(arguments)) as it is.
This is up for review precisely to get it nitpicked :) - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/#review69243 ----------------------------------------------------------- On Oct. 28, 2014, 5:44 a.m., David Narváez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120794/ > ----------------------------------------------------------- > > (Updated Oct. 28, 2014, 5:44 a.m.) > > > Review request for KDE Frameworks and rekonq. > > > Repository: rekonq > > > Description > ------- > > This is my humble attempt to implement the Unique Mode properly. I have been > trying to do this for the longest time in a way that avoids code duplication > but I can't find a way to jump over all the hurdles these API impose. I tried > learning from other ports from KUniqueApplication but a quick look at LXR > shows there are plenty of applications that blindly ported to Unique Mode but > didn't bother implementing activateRequested and the one I found that did was > plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the > code duplication is less evident. At this point, I would like someone who > knows about the QCommandLineParser + KDBusAddons dance to look at this and > tell if it is reasonable or not. > > The current patch just makes it possible to open several Rekonq applications. > It does not do the right thing when a Rekonq window is already open in the > current activity and a user clicks a link elsewhere (step 4 in the Testing > Done section) because it starts a brand new Rekonq window, but that's a > different patch. It also does some funky thing asking you if you want to > restore the previous session when nothing has crashed, I have to check that. > > > Diffs > ----- > > src/application.h 7ccd60d > src/application.cpp c7c297d > src/main.cpp 7592f7a > > Diff: https://git.reviewboard.kde.org/r/120794/diff/ > > > Testing > ------- > > 1. Open one Rekonq window > 2. Try opening Rekonq again > 3. Try opeining Rekonq from a command line with some URLs > 4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a > link somewhere (I click on the links at the title of the Konversation > channels I am in) > 5. Open rekonq from the console using rekonq --incognito > 6. Open rekonq from the console using reknoq --webapp twitter.com > > Before this patch, nothing happens in steps 2 - 6. After a first version of > this patch that does not avoid the QCommandLine parser if the argument list > is not empty, the window opened at 1 crashes because the activateRequested > signal passes an empty list of arguments - not even the binary name - so > QCommandLine parser dies. With this patch, every step opens a new window > properly, step 5 opens an incognito window and step 6 opens a webapp window > (simple window). > > > Thanks, > > David Narváez > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
