D26934: KCM/Autostart Add a model to separate logic from UI
Méven Car
noreply at phabricator.kde.org
Wed Feb 26 15:59:17 GMT 2020
meven marked an inline comment as done.
meven added inline comments.
INLINE COMMENTS
> ervin wrote in autostart.cpp:87
> Unrelated right? Where do this come from?
The current default size was not very appropriate.
I took the liberty of changing this as well.
Before:
F8133214: Screenshot_20200226_140516.png <https://phabricator.kde.org/F8133214>
> ervin wrote in autostartmodel.h:15
> I'd give it a less generic name or move it in a namespace this is an ambiguous call waiting to happen. :-)
>
> Also I'd move the implementation in the cpp file and drop the static.
>
> Finally, are you sure this is needed at all? I mean this is mostly about implicit conversion except that you provide a fallback in case of a missing value.
>
> If that's really needed I'd simplify the implementation by checking the index is between 0 and the last value of the enum in which case you can return it right away or otherwise return Xdg_Autostart.
> I'd give it a less generic name or move it in a namespace this is an ambiguous call waiting to happen. :-)
I renamed it to `AutostartModel::sourceFromInt`
> Also I'd move the implementation in the cpp file and drop the static.
I am sure of the point.
> Finally, are you sure this is needed at all? I mean this is mostly about implicit conversion except that you provide a fallback in case of a missing value.
> If that's really needed I'd simplify the implementation by checking the index is between 0 and the last value of the enum in which case you can return it right away or otherwise return Xdg_Autostart.
It makes the implementation quite explicit IMO, cannot return bad values, and can be enum-case checked by the compiler.
Checking against a range is less maintainable IMO.
(This is inspired by idiomatic rust, I am afraid)
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D26934
To: meven, mlaurent, ervin, #plasma, broulik, bport, crossi
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200226/4c985875/attachment-0001.html>
More information about the Plasma-devel
mailing list