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
