--===============2643817381314906355== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit > On May 26, 2014, 4:02 p.m., Frank Reininghaus wrote: > > Thanks for the patch. I think that we all agree that we should port away from kdelibs4support at some point, but is there a reason for doing it right now (i.e., does it fix a bug or cause some other noticeable improvement)? > > > > I'm asking because every large-scale change in the frameworks branch makes merging master into frameworks more painful. Merging is already non-trivial in some cases, due to the adjustments that were required to build with Qt5/KF5, and the replacement of all connect statements with the new syntax (but this at least fixed a few porting issues that would have been hard to spot otherwise, and which had to be fixed to make Dolphin work at all). > > > > This is just the reason why I proposed in https://git.reviewboard.kde.org/r/117395/ that only fixes for frameworks-specific bugs are done in frameworks branch for the time being. > > Emmanuel Pescosta wrote: > > only fixes for frameworks-specific bugs are done in frameworks branch for the time being. > Oh sorry, I missed that :( > Makes sense. > > Ok, I'll leave this review request open and when we start with the KF5 port I'll update this patch. Ok? > > Frank Reininghaus wrote: > Yes, updating it when we stop merging stuff from 4.x to frameworks makes sense! I hope that most of the KUrl -> QUrl transition was done in an automated way anyway, such that it will not be too much work to update it in the future. > > Aleix Pol Gonzalez wrote: > It's not automatic, the pain you're saving by not merging the patch you'll suffer it by trying to rebase it back. > > Frank Reininghaus wrote: > OK, so the argument is "the KUrl porting has been done already, so we have to suffer the merging pain, in order to prevent that the KUrl -> QUrl porting has to be done again"? If that is what most people think, feel free to push the patch once all issues are resolved, I don't want to be the one who forces people to throw away useful work. > > I really, really hope that people will start discussing any major changes BEFORE they start working on it at some point, but considering that I repeat this over and over again, and nobody cares, probably this hope is in vain :-( Coming up with a roadmap could be useful. - Aleix ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118314/#review58494 ----------------------------------------------------------- On May 25, 2014, 1:38 p.m., Emmanuel Pescosta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/118314/ > ----------------------------------------------------------- > > (Updated May 25, 2014, 1:38 p.m.) > > > Review request for Dolphin. > > > Repository: kde-baseapps > > > Description > ------- > > Ported Dolphin from KUrl to QUrl. > > Moved the "new name checking" from DolphinView::slotRoleEditingFinished to KFileItemModel::setData, > because we need the same url modifying code for both functions - avoid code duplication + porting mistakes. > > RenameDialog: > Changed renameItems to renameItem, we do the url modifying there for all two cases - > avoid code duplication + porting mistakes. > > > Diffs > ----- > > dolphin/src/dolphinapplication.cpp 8144da8 > dolphin/src/dolphincontextmenu.h 180f917 > dolphin/src/dolphincontextmenu.cpp 51351f0 > dolphin/src/dolphinmainwindow.h 1192f6e > dolphin/src/dolphinmainwindow.cpp 049c440 > dolphin/src/dolphinpart.h 9ab1e80 > dolphin/src/dolphinpart.cpp 479b809 > dolphin/src/dolphinpart_ext.h 5272df6 > dolphin/src/dolphinpart_ext.cpp fb7a4d2 > dolphin/src/dolphinviewcontainer.h a7a9969 > dolphin/src/dolphinviewcontainer.cpp 7610625 > dolphin/src/kitemviews/kfileitemmodel.h 62a283d > dolphin/src/kitemviews/kfileitemmodel.cpp 51bf546 > dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp acb3e0f > dolphin/src/kitemviews/kstandarditemlistwidget.cpp 7a9f31a > dolphin/src/kitemviews/private/kdirectorycontentscounter.cpp cd448e2 > dolphin/src/kitemviews/private/kfileitemclipboard.h 86eb8e9 > dolphin/src/kitemviews/private/kfileitemclipboard.cpp ebf50e2 > dolphin/src/kitemviews/private/kfileitemmodeldirlister.h da01d20 > dolphin/src/panels/folders/folderspanel.h 14d8e87 > dolphin/src/panels/folders/folderspanel.cpp 0b2ea38 > dolphin/src/panels/folders/treeviewcontextmenu.cpp 2e59ae8 > dolphin/src/panels/information/informationpanel.h c68b66e > dolphin/src/panels/information/informationpanel.cpp 4ad1276 > dolphin/src/panels/information/informationpanelcontent.h 67fdf6c > dolphin/src/panels/information/informationpanelcontent.cpp 9dc59db > dolphin/src/panels/information/phononwidget.h b5aedfe > dolphin/src/panels/information/phononwidget.cpp 4b2cc28 > dolphin/src/panels/panel.h a0b25d6 > dolphin/src/panels/panel.cpp 14b7c02 > dolphin/src/panels/places/placesitem.h 297f12d > dolphin/src/panels/places/placesitem.cpp 1729bbd > dolphin/src/panels/places/placesitemeditdialog.h bf34847 > dolphin/src/panels/places/placesitemeditdialog.cpp 0a66ef7 > dolphin/src/panels/places/placesitemmodel.h 4a374e5 > dolphin/src/panels/places/placesitemmodel.cpp 6ba91c5 > dolphin/src/panels/places/placespanel.h 16112e8 > dolphin/src/panels/places/placespanel.cpp d3614c9 > dolphin/src/panels/terminal/terminalpanel.h 987ee47 > dolphin/src/panels/terminal/terminalpanel.cpp 5c72652 > dolphin/src/search/dolphinsearchbox.h 53b12ff > dolphin/src/search/dolphinsearchbox.cpp df96f74 > dolphin/src/search/filenamesearchprotocol.h f691f99 > dolphin/src/search/filenamesearchprotocol.cpp b56a995 > dolphin/src/settings/applyviewpropsjob.h 68fdcc4 > dolphin/src/settings/applyviewpropsjob.cpp 9849216 > dolphin/src/settings/dolphinsettingsdialog.h 2de1950 > dolphin/src/settings/dolphinsettingsdialog.cpp 8df2996 > dolphin/src/settings/general/behaviorsettingspage.h 7a9c2f0 > dolphin/src/settings/general/behaviorsettingspage.cpp 7633b82 > dolphin/src/settings/general/generalsettingspage.h 0d28664 > dolphin/src/settings/general/generalsettingspage.cpp 8a8adf8 > dolphin/src/settings/startup/startupsettingspage.h 29cdc63 > dolphin/src/settings/startup/startupsettingspage.cpp 7f8c2d5 > dolphin/src/settings/viewpropertiesdialog.cpp 593d1c4 > dolphin/src/settings/viewpropsprogressinfo.h 6f8c763 > dolphin/src/settings/viewpropsprogressinfo.cpp a4a45da > dolphin/src/statusbar/dolphinstatusbar.h 4d6dbb2 > dolphin/src/statusbar/dolphinstatusbar.cpp 9f17c8e > dolphin/src/statusbar/spaceinfoobserver.h d2fb6eb > dolphin/src/statusbar/spaceinfoobserver.cpp 9125a93 > dolphin/src/statusbar/statusbarspaceinfo.h 1065d9f > dolphin/src/statusbar/statusbarspaceinfo.cpp 3692947 > dolphin/src/tests/kfileitemmodelbenchmark.cpp 66918b6 > dolphin/src/tests/kfileitemmodeltest.cpp 48e72e8 > dolphin/src/tests/testdir.h 0d3c5dd > dolphin/src/tests/testdir.cpp 8938e60 > dolphin/src/views/dolphinnewfilemenuobserver.h 3b5014a > dolphin/src/views/dolphinremoteencoding.h b317cc1 > dolphin/src/views/dolphinremoteencoding.cpp bf00e33 > dolphin/src/views/dolphinview.h 4ba1029 > dolphin/src/views/dolphinview.cpp 0e43dcd > dolphin/src/views/dolphinviewactionhandler.cpp 3955f25 > dolphin/src/views/draganddrophelper.h eda5fc5 > dolphin/src/views/draganddrophelper.cpp f8ae0ad > dolphin/src/views/renamedialog.h 29ef8bd > dolphin/src/views/renamedialog.cpp 3b94e01 > dolphin/src/views/versioncontrol/versioncontrolobserver.h 2c07b06 > dolphin/src/views/versioncontrol/versioncontrolobserver.cpp 9033e19 > dolphin/src/views/viewmodecontroller.h f476595 > dolphin/src/views/viewmodecontroller.cpp 26e1818 > dolphin/src/views/viewproperties.h 69b507f > dolphin/src/views/viewproperties.cpp bcea062 > > Diff: https://git.reviewboard.kde.org/r/118314/diff/ > > > Testing > ------- > > KFileItemModelTest fails. (Expanded items) > > > Thanks, > > Emmanuel Pescosta > > --===============2643817381314906355== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118314/

On May 26th, 2014, 4:02 p.m. UTC, Frank Reininghaus wrote:

Thanks for the patch. I think that we all agree that we should port away from kdelibs4support at some point, but is there a reason for doing it right now (i.e., does it fix a bug or cause some other noticeable improvement)?

I'm asking because every large-scale change in the frameworks branch makes merging master into frameworks more painful. Merging is already non-trivial in some cases, due to the adjustments that were required to build with Qt5/KF5, and the replacement of all connect statements with the new syntax (but this at least fixed a few porting issues that would have been hard to spot otherwise, and which had to be fixed to make Dolphin work at all).

This is just the reason why I proposed in https://git.reviewboard.kde.org/r/117395/ that only fixes for frameworks-specific bugs are done in frameworks branch for the time being.

On May 29th, 2014, 1:36 p.m. UTC, Emmanuel Pescosta wrote:

> only fixes for frameworks-specific bugs are done in frameworks branch for the time being.
Oh sorry, I missed that :(
Makes sense.

Ok, I'll leave this review request open and when we start with the KF5 port I'll update this patch. Ok?

On May 30th, 2014, 12:53 p.m. UTC, Frank Reininghaus wrote:

Yes, updating it when we stop merging stuff from 4.x to frameworks makes sense! I hope that most of the KUrl -> QUrl transition was done in an automated way anyway, such that it will not be too much work to update it in the future.

On May 30th, 2014, 1:38 p.m. UTC, Aleix Pol Gonzalez wrote:

It's not automatic, the pain you're saving by not merging the patch you'll suffer it by trying to rebase it back.

On May 30th, 2014, 1:46 p.m. UTC, Frank Reininghaus wrote:

OK, so the argument is "the KUrl porting has been done already, so we have to suffer the merging pain, in order to prevent that the KUrl -> QUrl porting has to be done again"? If that is what most people think, feel free to push the patch once all issues are resolved, I don't want to be the one who forces people to throw away useful work.

I really, really hope that people will start discussing any major changes BEFORE they start working on it at some point, but considering that I repeat this over and over again, and nobody cares, probably this hope is in vain :-(
Coming up with a roadmap could be useful.

- Aleix


On May 25th, 2014, 1:38 p.m. UTC, Emmanuel Pescosta wrote:

Review request for Dolphin.
By Emmanuel Pescosta.

Updated May 25, 2014, 1:38 p.m.

Repository: kde-baseapps

Description 

Ported Dolphin from KUrl to QUrl.

Moved the "new name checking" from DolphinView::slotRoleEditingFinished to KFileItemModel::setData,
because we need the same url modifying code for both functions - avoid code duplication + porting mistakes.

RenameDialog:
Changed renameItems to renameItem, we do the url modifying there for all two cases -
avoid code duplication + porting mistakes.

Testing 

KFileItemModelTest fails. (Expanded items)

Diffs

  • dolphin/src/dolphinapplication.cpp (8144da8)
  • dolphin/src/dolphincontextmenu.h (180f917)
  • dolphin/src/dolphincontextmenu.cpp (51351f0)
  • dolphin/src/dolphinmainwindow.h (1192f6e)
  • dolphin/src/dolphinmainwindow.cpp (049c440)
  • dolphin/src/dolphinpart.h (9ab1e80)
  • dolphin/src/dolphinpart.cpp (479b809)
  • dolphin/src/dolphinpart_ext.h (5272df6)
  • dolphin/src/dolphinpart_ext.cpp (fb7a4d2)
  • dolphin/src/dolphinviewcontainer.h (a7a9969)
  • dolphin/src/dolphinviewcontainer.cpp (7610625)
  • dolphin/src/kitemviews/kfileitemmodel.h (62a283d)
  • dolphin/src/kitemviews/kfileitemmodel.cpp (51bf546)
  • dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp (acb3e0f)
  • dolphin/src/kitemviews/kstandarditemlistwidget.cpp (7a9f31a)
  • dolphin/src/kitemviews/private/kdirectorycontentscounter.cpp (cd448e2)
  • dolphin/src/kitemviews/private/kfileitemclipboard.h (86eb8e9)
  • dolphin/src/kitemviews/private/kfileitemclipboard.cpp (ebf50e2)
  • dolphin/src/kitemviews/private/kfileitemmodeldirlister.h (da01d20)
  • dolphin/src/panels/folders/folderspanel.h (14d8e87)
  • dolphin/src/panels/folders/folderspanel.cpp (0b2ea38)
  • dolphin/src/panels/folders/treeviewcontextmenu.cpp (2e59ae8)
  • dolphin/src/panels/information/informationpanel.h (c68b66e)
  • dolphin/src/panels/information/informationpanel.cpp (4ad1276)
  • dolphin/src/panels/information/informationpanelcontent.h (67fdf6c)
  • dolphin/src/panels/information/informationpanelcontent.cpp (9dc59db)
  • dolphin/src/panels/information/phononwidget.h (b5aedfe)
  • dolphin/src/panels/information/phononwidget.cpp (4b2cc28)
  • dolphin/src/panels/panel.h (a0b25d6)
  • dolphin/src/panels/panel.cpp (14b7c02)
  • dolphin/src/panels/places/placesitem.h (297f12d)
  • dolphin/src/panels/places/placesitem.cpp (1729bbd)
  • dolphin/src/panels/places/placesitemeditdialog.h (bf34847)
  • dolphin/src/panels/places/placesitemeditdialog.cpp (0a66ef7)
  • dolphin/src/panels/places/placesitemmodel.h (4a374e5)
  • dolphin/src/panels/places/placesitemmodel.cpp (6ba91c5)
  • dolphin/src/panels/places/placespanel.h (16112e8)
  • dolphin/src/panels/places/placespanel.cpp (d3614c9)
  • dolphin/src/panels/terminal/terminalpanel.h (987ee47)
  • dolphin/src/panels/terminal/terminalpanel.cpp (5c72652)
  • dolphin/src/search/dolphinsearchbox.h (53b12ff)
  • dolphin/src/search/dolphinsearchbox.cpp (df96f74)
  • dolphin/src/search/filenamesearchprotocol.h (f691f99)
  • dolphin/src/search/filenamesearchprotocol.cpp (b56a995)
  • dolphin/src/settings/applyviewpropsjob.h (68fdcc4)
  • dolphin/src/settings/applyviewpropsjob.cpp (9849216)
  • dolphin/src/settings/dolphinsettingsdialog.h (2de1950)
  • dolphin/src/settings/dolphinsettingsdialog.cpp (8df2996)
  • dolphin/src/settings/general/behaviorsettingspage.h (7a9c2f0)
  • dolphin/src/settings/general/behaviorsettingspage.cpp (7633b82)
  • dolphin/src/settings/general/generalsettingspage.h (0d28664)
  • dolphin/src/settings/general/generalsettingspage.cpp (8a8adf8)
  • dolphin/src/settings/startup/startupsettingspage.h (29cdc63)
  • dolphin/src/settings/startup/startupsettingspage.cpp (7f8c2d5)
  • dolphin/src/settings/viewpropertiesdialog.cpp (593d1c4)
  • dolphin/src/settings/viewpropsprogressinfo.h (6f8c763)
  • dolphin/src/settings/viewpropsprogressinfo.cpp (a4a45da)
  • dolphin/src/statusbar/dolphinstatusbar.h (4d6dbb2)
  • dolphin/src/statusbar/dolphinstatusbar.cpp (9f17c8e)
  • dolphin/src/statusbar/spaceinfoobserver.h (d2fb6eb)
  • dolphin/src/statusbar/spaceinfoobserver.cpp (9125a93)
  • dolphin/src/statusbar/statusbarspaceinfo.h (1065d9f)
  • dolphin/src/statusbar/statusbarspaceinfo.cpp (3692947)
  • dolphin/src/tests/kfileitemmodelbenchmark.cpp (66918b6)
  • dolphin/src/tests/kfileitemmodeltest.cpp (48e72e8)
  • dolphin/src/tests/testdir.h (0d3c5dd)
  • dolphin/src/tests/testdir.cpp (8938e60)
  • dolphin/src/views/dolphinnewfilemenuobserver.h (3b5014a)
  • dolphin/src/views/dolphinremoteencoding.h (b317cc1)
  • dolphin/src/views/dolphinremoteencoding.cpp (bf00e33)
  • dolphin/src/views/dolphinview.h (4ba1029)
  • dolphin/src/views/dolphinview.cpp (0e43dcd)
  • dolphin/src/views/dolphinviewactionhandler.cpp (3955f25)
  • dolphin/src/views/draganddrophelper.h (eda5fc5)
  • dolphin/src/views/draganddrophelper.cpp (f8ae0ad)
  • dolphin/src/views/renamedialog.h (29ef8bd)
  • dolphin/src/views/renamedialog.cpp (3b94e01)
  • dolphin/src/views/versioncontrol/versioncontrolobserver.h (2c07b06)
  • dolphin/src/views/versioncontrol/versioncontrolobserver.cpp (9033e19)
  • dolphin/src/views/viewmodecontroller.h (f476595)
  • dolphin/src/views/viewmodecontroller.cpp (26e1818)
  • dolphin/src/views/viewproperties.h (69b507f)
  • dolphin/src/views/viewproperties.cpp (bcea062)

View Diff
--===============2643817381314906355==--