Replace all enum to enum class
Needs RevisionPublic
Actions

Authored by rominf on Feb 25 2018, 12:57 PM.

Details

Reviewers

Group Reviewers

Summary

Replace all enum to enum class. Rename some enum members where appropriate (for example: (RoleType)NameRole -> RoleType::Name).

Test Plan

Run all tests.
Check changes with your eyes.

Diff Detail

Repository

R318 Dolphin

Branch

enum-class

Lint

No Linters Available

Unit

No Unit Test Coverage

rominf created this revision.Feb 25 2018, 12:57 PM

Restricted Application added a subscriber: Dolphin. · View Herald TranscriptFeb 25 2018, 12:57 PM

rominf requested review of this revision.Feb 25 2018, 12:57 PM

rominf added a reviewer: Dolphin.Feb 25 2018, 12:58 PM

rominf added a project: Dolphin.

rominf edited the summary of this revision. (Show Details)Feb 25 2018, 1:15 PM

rominf edited the summary of this revision. (Show Details)Feb 25 2018, 1:19 PM

Thanks for the patch.

I agree we should use enum class for new code, I'm not sure it's worth porting to old enums though.
The problem is that this diff is huge and it will make git blame harder to use, for little gain.
@markg @rkflx toughts?

That said, if we really want to do it, please get rid of those static_casts where possible, they will only reduce the readability of the code.

src/dolphincontextmenu.cpp
69	If we change the type of `m_context`, we don't need the `static_cast`.

In D10832#213675, @elvisangelaccio wrote:

That said, if we really want to do it, please get rid of those static_casts where possible, they will only reduce the readability of the code.

Agreed.

In D10832#213675, @elvisangelaccio wrote:

Thanks for the patch.

I agree we should use enum class for new code, I'm not sure it's worth porting to old enums though.
The problem is that this diff is huge and it will make git blame harder to use, for little gain.
@markg @rkflx toughts?

That said, if we really want to do it, please get rid of those static_casts where possible, they will only reduce the readability of the code.

Tricky one!
On one side i'm all in for code improvements and modernization.
On the other hand, only if it makes sense.

As it's done now (with all the added static_casts's) it's imho not worth it as the new code looks less clean then the old one, but that can be fixed.

I personally would not bother making a patch for it, but now that the patch is there. Why not :)
New code should certainly use enum class!

Lastly, please do get the permission of the Dolphin maintainer as this is quite a refactoring.

Late to the party, here are my thoughts:

There's some value in the explicitness enum class sometimes provides, but counteracting it immediately with static_cast is not really useful.

I'd vote to apply this to new code only for now. Speaking from recent experience, it's easy to get things wrong regarding an enum refactoring ;) Better not to perform a mass change, unless there is a reviewer checking very thoroughly (I won't).

In D10832#213781, @rkflx wrote:

Late to the party, here are my thoughts:

There's some value in the explicitness enum class sometimes provides, but counteracting it immediately with static_cast is not really useful.

I'd vote to apply this to new code only for now. Speaking from recent experience, it's easy to get things wrong regarding an enum refactoring ;) Better not to perform a mass change, unless there is a reviewer checking very thoroughly (I won't).

Another variant is to apply only obvious changes and skip all changes that require static_casts. It's not hard to do (I need just filter my changes), easy to check for a reviewer (verify that (X)SomethingPostfix -> X::Something) and better than nothing.

In D10832#213795, @rominf wrote:

Another variant is to apply only obvious changes and skip all changes that require static_casts. It's not hard to do (I need just filter my changes), easy to check for a reviewer (verify that (X)SomethingPostfix -> X::Something) and better than nothing.

+1, sounds sane.

What about this diff? Will this be accepted if I revert all enums that require static_cast?

In D10832#218767, @rominf wrote:

What about this diff? Will this be accepted if I revert all enums that require static_cast?

Likely, yes. No promises though.

Remove almost all static_cast (only 5 is left!)

Fix compilation error

Please review.

Large review!
I was going to give a ship it till i compile tested it. It failed!
You can find the output here: https://p.sc2.nl/HJDBOJmtM

This revision now requires changes to proceed.Mar 11 2018, 5:33 PM

There is no reason to rush this in now, I'd suggest to wait the feature freeze on March 22 [1] so that we have enough time to be sure we don't break anything.

[1]: https://community.kde.org/Schedules/Applications/18.04_Release_Schedule

In D10832#223379, @markg wrote:

Large review!
I was going to give a ship it till i compile tested it. It failed!
You can find the output here: https://p.sc2.nl/HJDBOJmtM

It's strange. It builds even without additional warnings: https://gist.github.com/rominf/d3d55d85c23f83366a1ae7b5af525613

In D10832#223393, @elvisangelaccio wrote:

There is no reason to rush this in now, I'd suggest to wait the feature freeze on March 22 [1] so that we have enough time to be sure we don't break anything.

[1]: https://community.kde.org/Schedules/Applications/18.04_Release_Schedule

ping @rominf
It would probably need some rebasing after so long....

Restricted Application added a subscriber: kfm-devel. · View Herald TranscriptFeb 17 2020, 4:59 PM

rominf added a comment.Feb 17 2020, 5:11 PM

This comment was removed by rominf.

In D10832#613043, @meven wrote:

ping @rominf
It would probably need some rebasing after so long....

Hi. Unfortunately, I don't have time and desire to do that now.

Revision Contents
Changeset List

			Path	Packages
M			src/dolphincontextmenu.h (2 lines)
M			src/dolphincontextmenu.cpp (8 lines)
M			src/dolphinmainwindow.cpp (8 lines)
M			src/kitemviews/kfileitemlistview.cpp (2 lines)
M			src/kitemviews/kfileitemmodel.h (2 lines)
M			src/kitemviews/kfileitemmodel.cpp (10 lines)
M			src/kitemviews/kfileitemmodelrolesupdater.h (4 lines)
M			src/kitemviews/kfileitemmodelrolesupdater.cpp (70 lines)
M			src/kitemviews/kitemlistcontroller.h (6 lines)
M			src/kitemviews/kitemlistcontroller.cpp (50 lines)
M			src/kitemviews/kitemlistheader.cpp (6 lines)
M			src/kitemviews/kitemlistselectionmanager.h (6 lines)
M			src/kitemviews/kitemlistselectionmanager.cpp (16 lines)
M			src/kitemviews/kitemlistview.h (4 lines)
M			src/kitemviews/kitemlistview.cpp (60 lines)
M			src/kitemviews/kitemlistviewaccessible.cpp (2 lines)
M			src/kitemviews/kstandarditemlistview.h (8 lines)
M			src/kitemviews/kstandarditemlistview.cpp (18 lines)
M			src/kitemviews/kstandarditemlistwidget.h (8 lines)
M			src/kitemviews/kstandarditemlistwidget.cpp (44 lines)
M			src/kitemviews/private/kdirectorycontentscounter.cpp (8 lines)
M			src/kitemviews/private/kdirectorycontentscounterworker.h (2 lines)
M			src/kitemviews/private/kdirectorycontentscounterworker.cpp (4 lines)
M			src/kitemviews/private/kitemlistheaderwidget.h (6 lines)
M			src/kitemviews/private/kitemlistheaderwidget.cpp (22 lines)
M			src/kitemviews/private/kitemlistviewanimation.cpp (10 lines)
M			src/panels/folders/folderspanel.h (4 lines)
M			src/panels/folders/folderspanel.cpp (12 lines)
M			src/panels/information/pixmapviewer.h (8 lines)
M			src/panels/information/pixmapviewer.cpp (10 lines)
M			src/panels/places/placesitemmodel.cpp (10 lines)
M			src/panels/places/placespanel.cpp (2 lines)
M			src/settings/kcm/kcmdolphinviewmodes.cpp (6 lines)
M			src/settings/servicemodel.cpp (6 lines)
M			src/settings/viewmodes/dolphinfontrequester.h (2 lines)
M			src/settings/viewmodes/dolphinfontrequester.cpp (10 lines)
M			src/settings/viewmodes/viewmodesettings.h (8 lines)
M			src/settings/viewmodes/viewmodesettings.cpp (24 lines)
M			src/settings/viewmodes/viewsettingspage.cpp (6 lines)
M			src/settings/viewmodes/viewsettingstab.h (8 lines)
M			src/settings/viewmodes/viewsettingstab.cpp (48 lines)
M			src/settings/viewpropertiesdialog.cpp (14 lines)
M			src/tests/kitemlistcontrollertest.cpp (22 lines)
M			src/tests/kitemlistselectionmanagertest.cpp (32 lines)
M			src/tests/placesitemmodeltest.cpp (10 lines)
M			src/views/dolphinfileitemlistwidget.cpp (40 lines)
M			src/views/dolphinitemlistview.cpp (22 lines)
M			src/views/dolphinview.h (3 lines)
M			src/views/dolphinview.cpp (18 lines)
M			src/views/dolphinviewactionhandler.cpp (12 lines)
M			src/views/versioncontrol/kversioncontrolplugin.h (22 lines)
M			src/views/versioncontrol/versioncontrolobserver.cpp (4 lines)
M			src/views/viewproperties.cpp (20 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	28017	b95cc59		Feb 25 2018, 12:57 PM	★	★
Diff 2	28859	2523442	Remove almost all static_cast (only 5 is left!)	Mar 6 2018, 6:52 PM	★	★
Diff 3	28884	2523442	Fix compilation error	Mar 7 2018, 7:39 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
7d286ede7d76	161a70a19c43	2523442d4d67	Roman Inflianskas	Replace enum to enum class	Mar 6 2018, 6:06 PM

Replace all enum to enum classNeeds RevisionPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 28884

src/dolphincontextmenu.h

src/dolphincontextmenu.cpp

src/dolphinmainwindow.cpp

src/kitemviews/kfileitemlistview.cpp

src/kitemviews/kfileitemmodel.h

src/kitemviews/kfileitemmodel.cpp

src/kitemviews/kfileitemmodelrolesupdater.h

src/kitemviews/kfileitemmodelrolesupdater.cpp

src/kitemviews/kitemlistcontroller.h

src/kitemviews/kitemlistcontroller.cpp

src/kitemviews/kitemlistheader.cpp

src/kitemviews/kitemlistselectionmanager.h

src/kitemviews/kitemlistselectionmanager.cpp

src/kitemviews/kitemlistview.h

src/kitemviews/kitemlistview.cpp

src/kitemviews/kitemlistviewaccessible.cpp

src/kitemviews/kstandarditemlistview.h

src/kitemviews/kstandarditemlistview.cpp

src/kitemviews/kstandarditemlistwidget.h

src/kitemviews/kstandarditemlistwidget.cpp

src/kitemviews/private/kdirectorycontentscounter.cpp

src/kitemviews/private/kdirectorycontentscounterworker.h

src/kitemviews/private/kdirectorycontentscounterworker.cpp

src/kitemviews/private/kitemlistheaderwidget.h

src/kitemviews/private/kitemlistheaderwidget.cpp

src/kitemviews/private/kitemlistviewanimation.cpp

src/panels/folders/folderspanel.h

src/panels/folders/folderspanel.cpp

src/panels/information/pixmapviewer.h

src/panels/information/pixmapviewer.cpp

src/panels/places/placesitemmodel.cpp

src/panels/places/placespanel.cpp

src/settings/kcm/kcmdolphinviewmodes.cpp

src/settings/servicemodel.cpp

src/settings/viewmodes/dolphinfontrequester.h

src/settings/viewmodes/dolphinfontrequester.cpp

src/settings/viewmodes/viewmodesettings.h

src/settings/viewmodes/viewmodesettings.cpp

src/settings/viewmodes/viewsettingspage.cpp

src/settings/viewmodes/viewsettingstab.h

src/settings/viewmodes/viewsettingstab.cpp

src/settings/viewpropertiesdialog.cpp

src/tests/kitemlistcontrollertest.cpp

src/tests/kitemlistselectionmanagertest.cpp

src/tests/placesitemmodeltest.cpp

src/views/dolphinfileitemlistwidget.cpp

src/views/dolphinitemlistview.cpp

src/views/dolphinview.h

src/views/dolphinview.cpp

src/views/dolphinviewactionhandler.cpp

src/views/versioncontrol/kversioncontrolplugin.h

src/views/versioncontrol/versioncontrolobserver.cpp

src/views/viewproperties.cpp

Replace all enum to enum class
Needs RevisionPublic
Actions

Revision Contents
Changeset List