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

This revision was automatically updated to reflect the committed changes. Closed by commit R119:01776488ab8b: KCM/Autostart Add a model to separate logic from UI (authored by meven). REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26934?vs=80200&id=82533

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

meven added inline comments. INLINE COMMENTS > ervin wrote in autostartmodel.h:52 > Weeell... knowing about Qt::DisplayRole is kind of prerequisite to making > your own model. :-) As you want. IMO this is not a good pattern to expect reviewers to have any previous knowledge about enum values,

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

meven updated this revision to Diff 80200. meven added a comment. Use Qt::DisplayRole directly instead of using Roles::DisplayRole REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26934?vs=80196&id=80200 BRANCH D26934-2 REVISION DETAIL https://ph

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

meven updated this revision to Diff 80196. meven marked 11 inline comments as done. meven added a comment. Address review, fix an issue when adding a desktop item REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26934?vs=79639&id=80196 BRANCH D26934

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

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > autostart.cpp:167 > { > -KOpenWithDialog owdlg( this ); > -if (owdlg.exec() != QDialog::Accepted) > -return; > +KOpenWithDialog* owdlg = new

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

meven updated this revision to Diff 79639. meven marked 20 inline comments as done. meven added a comment. Make most dialogs use open/finished, make all dialogs modal... REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26934?vs=78855&id=79639 BRANCH

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

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > broulik wrote in autostart.cpp:182 > Yes. > It then also needs to set a transient parent and window modality manual I > think Yes, otherwise you wouldn't get a

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

ervin added inline comments. INLINE COMMENTS > meven wrote in autostartmodel.h:52 > I see, I meant my code to have all Roles used explicitly declared here rather > than relying on the developer knowing about Qt::DisplayRole. Weeell... knowing about Qt::DisplayRole is kind of prerequisite to mak

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

meven added inline comments. INLINE COMMENTS > broulik wrote in autostartmodel.h:52 > Why define `DisplayRole` as dedicated enum entry, rather than just using > `Qt::DisplayRole` in the code everywhere I see, I meant my code to have all Roles used explicitly declared here rather than relying o

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

broulik added inline comments. INLINE COMMENTS > meven wrote in autostart.cpp:210 > ? rows inserted gives you a range of [first,last] which was added > meven wrote in autostart.cpp:182 > You mean I should use open/finished right ? Yes. It then also needs to set a transient parent and window mo

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

meven added inline comments. INLINE COMMENTS > broulik wrote in autostart.cpp:210 > You probably want to loop > > for (int i = first; i <= last; ++i) { ? > broulik wrote in autostart.cpp:182 > In preparation for a move to QML, can we please not `exec()` You mean I should use open/finished r

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

meven updated this revision to Diff 78855. meven marked 10 inline comments as done. meven added a comment. code style mostly, adress Kai review REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26934?vs=78681&id=78855 BRANCH D26934 REVISION DETAIL

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

ervin requested changes to this revision. ervin added a comment. This revision now requires changes to proceed. Damn turns out I still missed a few, so what @broulik says (when it still applies). @broulik : are you sure about all your comments? It looks like something odd happened, some

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

broulik added inline comments. INLINE COMMENTS > autostart.cpp:182 > +AddScriptDialog addDialog(this); > +if (addDialog.exec() != QDialog::Accepted) { > +return; In preparation for a move to QML, can we please not `exec()` > autostart.cpp:192 > +QTreeWidgetItem* widgetItem =

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

ervin accepted this revision. ervin added a comment. This revision is now accepted and ready to land. Thanks for your patience :-) REPOSITORY R119 Plasma Desktop BRANCH D26934 REVISION DETAIL https://phabricator.kde.org/D26934 To: meven, mlaurent, ervin, #plasma, broulik, bport, cross

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

meven updated this revision to Diff 78681. meven marked 2 inline comments as done. meven added a comment. remove spurious space REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26934?vs=78680&id=78681 BRANCH D26934 REVISION DETAIL https://phabric

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

ervin requested changes to this revision. ervin added a comment. This revision now requires changes to proceed. One last nitpick INLINE COMMENTS > autostartmodel.cpp:121 > +case PlasmaStart: > +return static_cast (index); > +default: Please remove the spurious space before (

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

meven updated this revision to Diff 78680. meven added a comment. Remove spaces, use static_cast for less repetition REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26934?vs=78074&id=78680 BRANCH D26934 REVISION DETAIL https://phabricator.kde.or

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

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->ad

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

meven updated this revision to Diff 78074. meven marked 13 inline comments as done. meven added a comment. Address review, use Q_GLOBAL_STATIC_WITH_ARGS, move AutostartModel::sourceFromInt to cpp, code formatting REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricato

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

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > autostart.h:52 > +void updateDesktopStartItem(DesktopStartItem *item, const QString &name, > const QString &command, bool disabled, const QString &fileName)

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

meven marked an inline comment as done. meven added inline comments. INLINE COMMENTS > meven wrote in autostart.cpp:87 > The current default size was not very appropriate. > I took the liberty of changing this as well. > Before: > F8133214: Screenshot_20200226_140516.png >

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

meven updated this revision to Diff 77261. meven added a comment. Reversed resizing the window REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26934?vs=76470&id=77261 BRANCH arcpatch-D26934 REVISION DETAIL https://phabricator.kde.org/D26934 AFF

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

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_2020022

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

meven updated this revision to Diff 76470. meven marked 40 inline comments as done. meven edited the test plan for this revision. meven added a comment. Review processing, thanks @ervin ;) REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26934?vs=74413

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

ervin added inline comments. INLINE COMMENTS > autostart.cpp:87 > > +setMinimumHeight(300); > +setMinimumWidth(400); Unrelated right? Where do this come from? > autostart.cpp:112 > > -void Autostart::addItem( DesktopStartItem* item, const QString& name, const > QString& run, const

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

meven created this revision. meven added reviewers: mlaurent, ervin, Plasma. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. meven requested review of this revision. REVISION SUMMARY Adds the complete fileName of the autastart entries as tooltips. TEST PLAN - Add a de