[Differential] [Commented On] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.
jonathans added a comment. I wrote the first patch to be as minimal as possible and to be consistent with the previous coding style. I therefore left the early returns in place. I wrote the latest patch based on my interpretation of your (@kfunk) feedback that it makes more sense to test d->w than d->native. Because implementing this involved a change to the existing coding style I took the liberty of writing it according to what I believe to be good coding practice. Early returns are suitable for dealing with erroneous or trivial cases, but less so when dealing with modes of operation, as in this code. They interrupt the logical flow of the code, and because the conditionally executed code no longer sits inside an indented block, make it less evident to the reader that it is conditionally executed. Actually thinking about this code a bit more closely, I would now take the position that the earlier practice of only testing d->native is more sensible. What is happening here is that kfiledialog has two modes of operation: native and non-native. This mode is reflected in the two variables d->native and d->w, of which only one should ever be non-null. Which of the two variable the code tests now (ie with the latest patch) varies from case to case, which makes it inconsistent. Moreover the meaning of d->native is self-explanatory, unlike d->w. Although testing d->w would avoid segmentation faults if both d->native or d->w were null, the fact is that if that ever happened there would have to be a serious problem anyway and the code would be obviously broken. Trapping a segfault with a debugger would actually make it easier to locate the problem than, for instance, a file dialog window simply failing to be displayed. But that's all going off on a tangent. Whatever your call is on this case I'm happy to accept. REPOSITORY R239 KDELibs4Support REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks, dfaure, kfunk Cc: kfunk, aacid
[Differential] [Commented On] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.
kfunk added a comment. Why did you remove all the early-returns? Was that the case before in one of your earlier patches? REPOSITORY R239 KDELibs4Support REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks, dfaure, kfunk Cc: kfunk, aacid
[Differential] [Commented On] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.
jonathans added a comment. Apparently updating the diff has had the side-effect of: > https://phabricator.kde.org/source/kdelibs4support/ KDELibs4Support as the repository for this revision. Please excuse my bumbling - I don't know why that happened or how to repair it. :( REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks, dfaure, kfunk Cc: kfunk, aacid
[Differential] [Commented On] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.
kfunk added a comment. In https://phabricator.kde.org/D2075#66751, @jonathans wrote: > Agreed that would be more robust. In writing the patch I was seeking consistency with those functions that already did the test, so those would also need to be updated. Are there any situations where the two tests would yield a different result, ie d->native is true and d->w is non-null? Probably not. But I think it makes more sense to check the pointer you're actually going to dereference in the next statement. Could you update the patch? And also fix the `nullptr` issues? REPOSITORY R239 KDELibs4Support REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks, dfaure, kfunk Cc: kfunk, aacid
[Differential] [Commented On] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.
jonathans added a comment. Agreed that would be more robust. In writing the patch I was seeking consistency with those functions that already did the test, so those would also need to be updated. Are there any situations where the two tests would yield a different result, ie d->native is true and d->w is non-null? REPOSITORY R239 KDELibs4Support REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks, dfaure, kfunk Cc: kfunk, aacid
[Differential] [Commented On] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.
aacid added a comment. Again we have a patch against no repo, so i don't even know how to read this patch because i can't read the rest of the file. INLINE COMMENTS > kfiledialog.cpp:879 > +return 0; > +} > return d->w->okButton(); nullptr here and in other places REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks, dfaure Cc: aacid
