> On March 7, 2016, 9:29 p.m., Gregor Mi wrote: > > src/kmoretools/kmoretoolsmenufactory.cpp, lines 128-129 > > <https://git.reviewboard.kde.org/r/127237/diff/2/?file=448481#file448481line128> > > > > Thanks for the comment. However, as long as we don't know the root > > cause for the null pointers, I would feel better if the comment states > > clearly that we don't know what is happening and that it is happening on > > Windows only. > > Kåre Särs wrote: > This one is not needed at the moment (I did not get null pointers here), > but I left it there just in case. I feel it is better to loose functionality > than getting a crash ;)
Then it's ok. General question: is there a common log file where one could write a message in case there is nullptr which was catched? > On March 7, 2016, 9:29 p.m., Gregor Mi wrote: > > src/kmoretools/kmoretoolsmenufactory.cpp, lines 237-238 > > <https://git.reviewboard.kde.org/r/127237/diff/2/?file=448481#file448481line237> > > > > see my comment above > > Kåre Särs wrote: > same as above ;) :) > On March 7, 2016, 9:29 p.m., Gregor Mi wrote: > > src/kmoretools/kmoretoolspresets.cpp, line 97 > > <https://git.reviewboard.kde.org/r/127237/diff/2/?file=448482#file448482line97> > > > > Again, this should not happen here, so please add a comment that this > > is a fix for Windows. > > Kåre Särs wrote: > This is the first place where we get the null pointers. > KMoreTools::registerServiceByDesktopEntryName() has at least two code paths > that return nullptr. > kmoretools.cpp > line 129: qCritical() << ... the kmt-desktopfile " << desktopEntryName << > " is provided but no Exec line is specified ... > line 130: return nullptr; > and > line 146: qCritical() << ... a kmt-desktopfile must be provided. Please > fix. Return nullptr. ... > line 147: return nullptr; > > These might be errors, but I would rather see that it does not crash even > if a runtime file is missing. I suspect that it is the later that generates > the nullptr. > This is why I check the returned pointer before using it. > > QStandardPaths::GenericDataLocation does not return ../share/ on Windows > and thus does not find the file Please add these findings to your code comments. Then Ship it. > On March 7, 2016, 9:29 p.m., Gregor Mi wrote: > > src/kmoretools/kmoretoolspresets.cpp, lines 165-167 > > <https://git.reviewboard.kde.org/r/127237/diff/2/?file=448482#file448482line165> > > > > As above: as long as we don't know the reason for the nullpointers I > > would be happier if there was a comment. > > > > That said I don't know what the KDE policy is when dealing with this > > kind of problem. Just adding null checks seems to make the code more > > complicated to analyse later. You are a long-term committer, so your words > > have a high weight. On the other hand I would be interested in a second > > opinion. > > Kåre Särs wrote: > KMoreToolsPresets::registerServiceByDesktopEntryName() has one code path > that returns a direct nullptr and another that calls > KMoreTools::registerServiceByDesktopEntryName() that also can return a > nullptr. So I cannot be sure that the pointer isn't null. So check it and > don't add nullptr ;) > > The problem is, with 99% certainty, the missing .desktop file, but IMO we > should not crash even if a runtime file is missing ;) I agree that a missing desktop file should not be reason for a runtime crash. I wonder if there is a good way to log those errors instead of silently do nothing. - Gregor ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127237/#review93270 ----------------------------------------------------------- On March 7, 2016, 8:38 p.m., Kåre Särs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127237/ > ----------------------------------------------------------- > > (Updated March 7, 2016, 8:38 p.m.) > > > Review request for KDE Frameworks and Gregor Mi. > > > Repository: knewstuff > > > Description > ------- > > On Windows we sometimes get null-pointers in stead of pointers to > KMoreToolsService. This patch adds checks for null-pointers in those places > that made Kate crash in the project plugin and adds a nullptr check before > adding them to the list. > > > Diffs > ----- > > src/kmoretools/kmoretoolsmenufactory.cpp 30f4d02 > src/kmoretools/kmoretoolspresets.cpp 2405321 > > Diff: https://git.reviewboard.kde.org/r/127237/diff/ > > > Testing > ------- > > Compiled and run on windows. No crashes in Kate when right-clicking files in > the project plugin. > > > Thanks, > > Kåre Särs > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
