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