Review Request 117478: Convert dolphin (frameworks) to Qt5 signal/slot syntax
Alexander Richardson
arichardson.kde at googlemail.com
Wed Apr 30 18:16:14 BST 2014
> On April 14, 2014, 10:30 p.m., Frank Reininghaus wrote:
> > Thanks for your work on this, Alexander! I'm glad that we are getting closer to a working KF5-based Dolphin.
> >
> > I'm a bit concerned by Thiago's comment though (thanks Thiago for the hint btw - are there details about the behavior differences between the two ways to establishe connections in Qt's docs? I couldn't find anything with a quick search). If there are slight behavior differences, then there might be a risk that we will see regressions. Since it is basically impossible to test the effect of all connections exhaustively, and I don't know exactly what the behavior difference is and if it could cause trouble, I'm not sure if we should really do an automated conversion of all connect statements. Maybe one should discuss this issue (which will be of interest not only to Dolphin, but also to many other apps and to KF5 itself) on the kde-frameworks mailing list?
> >
> > The safer solution would be to leave all existing connections as they are, unless they don't work, and use the new syntax only for new code. In principle, it should be easy to find out which connections don't work and cause Dolphin to not show any files because there is a runtime warning for each failed connect call, right?
>
> Alexander Richardson wrote:
> I didn't see see Thiago's message since I am not subscribed to kfm-devel. From what I saw in the Qt documentation I always assumed they were equivalent (except for not allowing default arguments in the slot), which other differences exist?
>
> The reason I changed it all to the new syntax was in order to get an error at compile time instead of at runtime. Fixing all theses issues when they occur at runtime sounds rather painful to me since it may only happen after pressing a certain button or navigating to a special folder. Obviously it is easy to fix due to the runtime warning, however it may require quite a few iterations.
>
> With the new syntax I didn't find any issues in my (admittedly light) testing by browsing through a few folders.
>
> Frank Reininghaus wrote:
> I agree that getting errors at compile time is more convenient, and that debugging all failed connections by looking at runtime error messages can be painful. But still, I think that it is impossible to test every single connection in the code base to verify that the behavior change does not cause any problems, and releasing a KF5-based Dolphin that might have subtle regressions in odd corner cases is something that I would like to avoid.
>
> Therefore, before we consider changing everything, I would prefer
>
> (a) That we get some clarity on how exactly the behavior of the two ways to establish connections is different, and
>
> (b) that the issue is discussed on the kde-frameworks mailing list, and that consensus is reached that making such a large-scale change in applications is desirable, because this is in no way Dolphin-specific, and the script that you used to make the change can also be used for other applications and libraries if people agree that the benefits outweigh the possible risks.
>
> I'm willing to help with debugging the runtime error messages. I hope that I'll find some time for that until the end of the week.
Merged this into review 117395 since git rebase -i causes loads of merge issues
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117478/#review55777
-----------------------------------------------------------
On April 10, 2014, 4:57 p.m., Alexander Richardson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117478/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 4:57 p.m.)
>
>
> Review request for Dolphin.
>
>
> Repository: kde-baseapps
>
>
> Description
> -------
>
> Requires https://git.reviewboard.kde.org/r/117395/
>
> Most of this conversion was done automatically, only minor manual changes were needed.
> There is a problem with KAction -> QAction, since there is no longer the signal triggered() that reports the middle mouse button.
> I attempted to fix this by using QApplication::mouseButtons(), however the triggered signal is emitted after the mouse is released, so we don't get that reported anymore. Will have to investigate more how this can be fixed. This issue affects middle mouse clicking on any toolbar button. Other than that dolphin seems to work fine.
>
> The terminal view signals could not be converted since there is no header file available that defines that signal (-> cannot get the function pointer).
>
>
> Diffs
> -----
>
> dolphin/src/dolphincontextmenu.cpp f295de7
> dolphin/src/dolphinmainwindow.h cb97612
> dolphin/src/dolphinmainwindow.cpp 8473014
> dolphin/src/dolphinpart.cpp 9081731
> dolphin/src/dolphinremoveaction.cpp 7d7c2f0
> dolphin/src/dolphinviewcontainer.cpp 768fd5e
> dolphin/src/filterbar/filterbar.cpp 6de6fbe
> dolphin/src/kitemviews/kfileitemlistview.cpp fd01f2c
> dolphin/src/kitemviews/kfileitemmodel.cpp fd773e1
> dolphin/src/kitemviews/kfileitemmodelrolesupdater.h a9e979a
> dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp 0865d40
> dolphin/src/kitemviews/kitemlistcontainer.cpp 8498286
> dolphin/src/kitemviews/kitemlistcontroller.cpp 7344b99
> dolphin/src/kitemviews/kitemlistheader.cpp e89ece0
> dolphin/src/kitemviews/kitemlistview.cpp 82f8a20
> dolphin/src/kitemviews/kitemlistwidget.cpp 85cd70c
> dolphin/src/kitemviews/kstandarditemlistwidget.cpp 9a9a734
> dolphin/src/kitemviews/private/kdirectorycontentscounter.cpp 65afb7c
> dolphin/src/kitemviews/private/kfileitemclipboard.cpp faace2a
> dolphin/src/kitemviews/private/kitemlistheaderwidget.cpp 1f210ab
> dolphin/src/kitemviews/private/kitemlistroleeditor.cpp 0a48f1b
> dolphin/src/kitemviews/private/kitemlistsmoothscroller.cpp 491461b
> dolphin/src/kitemviews/private/kitemlistviewanimation.cpp e347c5b
> dolphin/src/panels/folders/folderspanel.cpp 46c1b34
> dolphin/src/panels/folders/treeviewcontextmenu.cpp fa8844d
> dolphin/src/panels/information/informationpanel.cpp eda74f3
> dolphin/src/panels/information/informationpanelcontent.cpp b2dd158
> dolphin/src/panels/information/phononwidget.cpp a36ada1
> dolphin/src/panels/information/pixmapviewer.cpp 8a752c5
> dolphin/src/panels/places/placesitem.cpp 41f22cc
> dolphin/src/panels/places/placesitemeditdialog.cpp 08c910d
> dolphin/src/panels/places/placesitemmodel.cpp baa770d
> dolphin/src/panels/places/placespanel.cpp d5308ea
> dolphin/src/panels/terminal/terminalpanel.cpp bfd3002
> dolphin/src/search/dolphinfacetswidget.cpp b7315a0
> dolphin/src/search/dolphinsearchbox.cpp c178c43
> dolphin/src/search/filenamesearchprotocol.cpp 4d6f59f
> dolphin/src/settings/additionalinfodialog.cpp e9d5f60
> dolphin/src/settings/applyviewpropsjob.cpp 4bc77ca
> dolphin/src/settings/dolphinsettingsdialog.cpp 609e2ab
> dolphin/src/settings/general/behaviorsettingspage.cpp cbbde1d
> dolphin/src/settings/general/configurepreviewplugindialog.cpp 3ca08df
> dolphin/src/settings/general/confirmationssettingspage.cpp ab23a19
> dolphin/src/settings/general/generalsettingspage.cpp 18e1528
> dolphin/src/settings/general/previewssettingspage.cpp 38b61b9
> dolphin/src/settings/general/statusbarsettingspage.cpp 48622ac
> dolphin/src/settings/kcm/kcmdolphingeneral.cpp 26cb580
> dolphin/src/settings/kcm/kcmdolphinnavigation.cpp 36345a5
> dolphin/src/settings/kcm/kcmdolphinservices.cpp 6d8c761
> dolphin/src/settings/kcm/kcmdolphinviewmodes.cpp a7a9db3
> dolphin/src/settings/navigation/navigationsettingspage.cpp 8076d70
> dolphin/src/settings/serviceitemdelegate.cpp 7538e03
> dolphin/src/settings/services/servicessettingspage.cpp 48e816b
> dolphin/src/settings/startup/startupsettingspage.cpp 6938263
> dolphin/src/settings/trash/trashsettingspage.cpp cd69985
> dolphin/src/settings/viewmodes/dolphinfontrequester.cpp 6cb7b99
> dolphin/src/settings/viewmodes/viewsettingspage.cpp 4f8a3f0
> dolphin/src/settings/viewmodes/viewsettingstab.cpp bc12451
> dolphin/src/settings/viewpropertiesdialog.cpp 574f8e1
> dolphin/src/settings/viewpropsprogressinfo.cpp 9b7797d
> dolphin/src/statusbar/dolphinstatusbar.cpp 671ef4f
> dolphin/src/statusbar/statusbarspaceinfo.cpp 61b2833
> dolphin/src/views/dolphinnewfilemenuobserver.h 239476e
> dolphin/src/views/dolphinnewfilemenuobserver.cpp 7669f15
> dolphin/src/views/dolphinremoteencoding.cpp 04b350e
> dolphin/src/views/dolphinview.cpp 9f5f48a
> dolphin/src/views/dolphinviewactionhandler.h e80ffc0
> dolphin/src/views/dolphinviewactionhandler.cpp 48ec95c
> dolphin/src/views/renamedialog.cpp d8dbd77
> dolphin/src/views/tooltips/filemetadatatooltip.cpp b726996
> dolphin/src/views/tooltips/tooltipmanager.cpp bd69483
> dolphin/src/views/versioncontrol/versioncontrolobserver.cpp 36345d9
>
> Diff: https://git.reviewboard.kde.org/r/117478/diff/
>
>
> Testing
> -------
>
> After converting all the signal/slot connections, dolphin seems to work normally.
>
> I got a crash when moving something to trash, but I think this is because I currently don't have kio-trash on my system.
>
>
> Thanks,
>
> Alexander Richardson
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140430/bae18b41/attachment.htm>
More information about the kfm-devel
mailing list