D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> autostartitem.cpp:60
> +    m_comboBoxStartup->addItem( AutostartModel::listPathName()[1], AutostartEntrySource::PlasmaShutdown);
> +    m_comboBoxStartup->addItem( AutostartModel::listPathName()[2], AutostartEntrySource::PlasmaStart);
> +

Remove the space after the opening parenthesis on those three lines.

It's much better like this BTW, the loop wasn't indeed buying us much for just three values.

> autostartmodel.cpp:116
> +{
> +    switch (index) {
> +    case XdgAutoStart: return XdgAutoStart;

Heh, so we're playing who's the most stubborn here? I still find that "switch not saying it's a static_cast" really ugly and non idiomatic.

> autostartmodel.cpp:122
> +    }
> +    Q_UNREACHABLE();
> +    return XdgAutoStart;

I'd still advise getting rid of the switch but if it doesn't disappear at least move that to the "default:" case to silence potential warning on type's non exhaustion.

But really... static_cast needs to appear please. Alternatively to the if I was proposing you could also do:

  switch (index) {
  case XdgAutoStart:
  case XdgScripts:
  case PlasmaShutdown:
  case PlasmaStart:
      return static_cast<AutostartEntrySource>(index);
  default:
      Q_UNREACHABLE();
  }
  
  return XdgAutoStart;

At least it'd be DRY and it'd be more obvious we got a cast...

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/20200327/37ad0fa1/attachment-0001.html>