D26895: Introduce an Info Center mode

[Ovidiu Chi]

ovichiro added a comment.


  In D26895#605603 , @ngraham wrote:
  
  > Sounds sensible. Would you like to submit a patch?
  
  
  I don't know how yet. I need to brush up on C++ first. I'll see what I can do 
in the next few days or weeks.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma, davidedmundson
Cc: ngraham, ovichiro, meven, davidedmundson, broulik, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Nathaniel Graham]

ngraham added a comment.


  In D26895#605255 , @ovichiro wrote:
  
  > Not sure it's my place to do so, but I'd like to make a small comment.  
  >  Instead of passing `mode` around and all the `if` statements, wouldn't it 
be better to have a single decision point in `main.cpp`, like a factory class?  
  >  Use polymorphism and create something like `InfoCenter : CommonParent`, 
`SystemSettings : CommonParent`, each knowing how to initialize their 
`KAboutData`, GUI elements, icons etc.?  
  >  And in `main.cpp` do something like `CommonParent * commonParent = new 
InfoCenter() // or new SystemSettings()` and use that instead of `mode`.  
  >  Further down the road, if you need to add something to system settings, 
you could do it in the SystemSettings class maybe and not have to add another 
`if` to remove it from the InfoCenter part. Just a thought, not sure if it fits.
  
  
  Sounds sensible. Would you like to submit a patch?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma, davidedmundson
Cc: ngraham, ovichiro, meven, davidedmundson, broulik, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Ovidiu Chi]

ovichiro added inline comments.

INLINE COMMENTS

> main.cpp:68
> +aboutData = KAboutData(QStringLiteral("systemsettings"), 
> i18n("System Settings"), QLatin1String(PROJECT_VERSION), i18n("Central 
> configuration center by KDE."), KAboutLicense::GPL, i18n("(c) 2009, Ben 
> Cooksley"));
> +aboutData.addAuthor(i18n("Ben Cooksley"), i18n("Maintainer"), 
> QStringLiteral("bcooks...@kde.org"));
> +aboutData.addAuthor(i18n("Mathias Soeken"), i18n("Developer"), 
> QStringLiteral("msoe...@informatik.uni-bremen.de"));

The authors should maybe be moved outside if-else since they are the same for 
both branches.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma, davidedmundson
Cc: ovichiro, meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Ovidiu Chi]

ovichiro added a comment.


  Not sure it's my place to do so, but I'd like to make a small comment.  
  Instead of passing `mode` around and all the `if` statements, wouldn't it be 
better to have a single decision point in `main.cpp`, like a factory class?  
  Use polymorphism and create something like `InfoCenter : CommonParent`, 
`SystemSettings : CommonParent`, each knowing how to initialize their 
`KAboutData`, GUI elements, icons etc.?  
  And in `main.cpp` do something like `CommonParent * commonParent = new 
InfoCenter() // or new SystemSettings()` and use that instead of `mode`.  
  Further down the road, if you need to add something to system settings, you 
could do it in the SystemSettings class maybe and not have to add another `if` 
to remove it from the InfoCenter part. Just a thought, not sure if it fits.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma, davidedmundson
Cc: ovichiro, meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Marco Martin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R124:ca28b62000ff: Introduce an Info Center mode (authored by 
mart).

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26895?vs=74491=74492

REVISION DETAIL
  https://phabricator.kde.org/D26895

AFFECTED FILES
  app/SettingsBase.cpp
  app/SettingsBase.h
  app/main.cpp
  core/BaseData.cpp
  core/BaseData.h
  core/BaseMode.cpp
  core/BaseMode.h
  core/MenuItem.cpp
  core/MenuModel.cpp
  core/MenuModel.h
  core/ModuleView.cpp
  core/ModuleView.h
  icons/IconMode.cpp
  sidebar/SidebarMode.cpp
  sidebar/SidebarMode.h

To: mart, #plasma, davidedmundson
Cc: meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Marco Martin]

mart updated this revision to Diff 74491.
mart added a comment.


  - better todo message

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26895?vs=74487=74491

BRANCH
  mart/kinfoCenterMode

REVISION DETAIL
  https://phabricator.kde.org/D26895

AFFECTED FILES
  app/SettingsBase.cpp
  app/SettingsBase.h
  app/main.cpp
  core/BaseData.cpp
  core/BaseData.h
  core/BaseMode.cpp
  core/BaseMode.h
  core/MenuItem.cpp
  core/MenuModel.cpp
  core/MenuModel.h
  core/ModuleView.cpp
  core/ModuleView.h
  icons/IconMode.cpp
  sidebar/SidebarMode.cpp
  sidebar/SidebarMode.h

To: mart, #plasma, davidedmundson
Cc: meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> SidebarMode.cpp:589
>  
> -if (introPageVisible) {
> -d->activeCategoryRow = -1;
> -emit activeCategoryRowChanged();
> -d->activeSubCategoryRow = -1;
> -emit activeSubCategoryRowChanged();
> -d->placeHolderWidget->show();
> -d->moduleView->hide();
> -} else {
> +// TODO: eventually make the intro page of SystemSettings a KCM as well?
> +if (homeItem()) {

Yes! that would be a nicer design

REPOSITORY
  R124 System Settings

BRANCH
  mart/kinfoCenterMode

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma, davidedmundson
Cc: meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Marco Martin]

mart updated this revision to Diff 74487.
mart added a comment.


  - use mode

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26895?vs=74481=74487

BRANCH
  mart/kinfoCenterMode

REVISION DETAIL
  https://phabricator.kde.org/D26895

AFFECTED FILES
  app/SettingsBase.cpp
  app/SettingsBase.h
  app/main.cpp
  core/BaseData.cpp
  core/BaseData.h
  core/BaseMode.cpp
  core/BaseMode.h
  core/MenuItem.cpp
  core/MenuModel.cpp
  core/MenuModel.h
  core/ModuleView.cpp
  core/ModuleView.h
  icons/IconMode.cpp
  sidebar/SidebarMode.cpp
  sidebar/SidebarMode.h

To: mart, #plasma
Cc: meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> main.cpp:81
>  
> -SettingsBase *mainWindow = new SettingsBase();
> +if (binaryName == QStringLiteral("kinfocenter")) {
> +aboutData.setDesktopFileName(QStringLiteral("org.kde.kinfocenter"));

You can use mode here

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma
Cc: meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Marco Martin]

mart updated this revision to Diff 74481.
mart added a comment.


  - remove usekless qdebug

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26895?vs=74422=74481

BRANCH
  mart/kinfoCenterMode

REVISION DETAIL
  https://phabricator.kde.org/D26895

AFFECTED FILES
  app/SettingsBase.cpp
  app/SettingsBase.h
  app/main.cpp
  core/BaseData.cpp
  core/BaseData.h
  core/BaseMode.cpp
  core/BaseMode.h
  core/MenuItem.cpp
  core/MenuModel.cpp
  core/MenuModel.h
  core/ModuleView.cpp
  core/ModuleView.h
  icons/IconMode.cpp
  sidebar/SidebarMode.cpp
  sidebar/SidebarMode.h

To: mart, #plasma
Cc: meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> BaseMode.cpp:32
>  #include "ModuleView.h"
> -
> +#include 
>  class BaseMode::Private {

Not needed

> mart wrote in ModuleView.cpp:349
> that's a good point.
> Tough, at the moment we don't do anything with the statistics in kinfocenter 
> mode, so for now better not clutter the kastats database?

We can ignore this for now, for sure, since we don't have a use case indeed.
KFeedback may like to access kactivities stats though.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma
Cc: meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Marco Martin]

mart updated this revision to Diff 74422.
mart marked 6 inline comments as done.
mart added a comment.


  - base behavior on executable name

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26895?vs=74307=74422

BRANCH
  mart/kinfoCenterMode

REVISION DETAIL
  https://phabricator.kde.org/D26895

AFFECTED FILES
  app/SettingsBase.cpp
  app/SettingsBase.h
  app/main.cpp
  core/BaseData.cpp
  core/BaseData.h
  core/BaseMode.cpp
  core/BaseMode.h
  core/MenuItem.cpp
  core/MenuModel.cpp
  core/MenuModel.h
  core/ModuleView.cpp
  core/ModuleView.h
  icons/IconMode.cpp
  sidebar/SidebarMode.cpp
  sidebar/SidebarMode.h

To: mart, #plasma
Cc: meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> meven wrote in ModuleView.cpp:349
> We might want to have a different url scheme like "kinfo" insteat of "kcm" 
> and "org.kde.kinfocenter" instead of "org.kde.systemsettings"

that's a good point.
Tough, at the moment we don't do anything with the statistics in kinfocenter 
mode, so for now better not clutter the kastats database?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma
Cc: meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> MenuModel.cpp:26
>  #include "MenuItem.h"
> -
> +#include 
>  

not needed

> ModuleView.cpp:349
>  if (activeModule) {
> -
> KActivities::ResourceInstance::notifyAccessed(QUrl(QStringLiteral("kcm:") + 
> activeModule->moduleInfo().service()->storageId()),
> -QStringLiteral("org.kde.systemsettings"));
> +if (d->mSaveStatistics) {
> +
> KActivities::ResourceInstance::notifyAccessed(QUrl(QStringLiteral("kcm:") + 
> activeModule->moduleInfo().service()->storageId()),

We might want to have a different url scheme like "kinfo" insteat of "kcm" and 
"org.kde.kinfocenter" instead of "org.kde.systemsettings"

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma
Cc: meven, davidedmundson, broulik, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> SettingsBase.h:48
> +// This has effect only if set right after the ctor
> +void setInfoCenterMode(bool set);
>  bool queryClose() override;

why not an arg in the ctor then?

> SettingsBase.h:95
> +MenuItem * homeModule = nullptr;
>  MenuItem * lostFound = nullptr;
>  KService::List categories;

what happens to lost and found modules

> main.cpp:57
>  QCommandLineParser parser;
> +QCommandLineOption infoCenterOption(QStringList() << QStringLiteral("i") 
> << QStringLiteral("showInformation"),
> +i18n("Show System information 
> instead of System configuration."));

As an alternative, we could switch based on our binary name, from arg[0] and 
then install a symlink.
Then we have 100% compatibility

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma
Cc: davidedmundson, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26895: Introduce an Info Center mode

[Kai Uwe Broulik]

broulik added a comment.


  Great idea!

INLINE COMMENTS

> SettingsBase.h:100
>  KAboutApplicationDialog * aboutDialog = nullptr;
> +bool m_infoCenterMode = false;
>  };

I would prefer this to be an enum

> main.cpp:66
> +if (infoCenterMode) {
> +QCoreApplication::setApplicationName(QStringLiteral("kinfocenter"));
> +application.setApplicationName(QStringLiteral("kinfocenter"));

Can we not just create a different `KAboutData` instance?

> main.cpp:71
> +aboutData.setDesktopFileName(QStringLiteral("org.kde.kinfocenter"));
> +aboutData.setShortDescription(QStringLiteral("Centralized and 
> convenient overview of system information"));
> +
> application.setWindowIcon(QIcon::fromTheme(QStringLiteral("hwinfo")));

`i18n`

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D26895

To: mart, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26895: Introduce an Info Center mode

[Marco Martin]

mart created this revision.
mart added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
mart requested review of this revision.

REVISION SUMMARY
  withthe command line switch -i, start
  systemsettings loading the KInfoCenter modules and hyerarchy instead.
  
  - KAboutData and dbus service name will be the same of KInfoCenter
  - The startup page is not shown but the "System Information" kcm is loaded 
instead
  - KACtivityStats is disabled
  - The home button will load the "System Information" kcm as well
  - It will *always* be in sidebar mode regardless how systemsettings is 
configured
  - Config dialog can't be open.

TEST PLAN
  - SystemSettings mode functionality didn't change
  - infocenter mode is fully functional

REPOSITORY
  R124 System Settings

BRANCH
  mart/kinfoCenterMode

REVISION DETAIL
  https://phabricator.kde.org/D26895

AFFECTED FILES
  app/SettingsBase.cpp
  app/SettingsBase.h
  app/SystemSettingsApp.cpp
  app/SystemSettingsApp.h
  app/main.cpp
  core/BaseData.cpp
  core/BaseData.h
  core/BaseMode.cpp
  core/BaseMode.h
  core/MenuItem.cpp
  core/MenuModel.cpp
  core/MenuModel.h
  core/ModuleView.cpp
  core/ModuleView.h
  sidebar/SidebarMode.cpp

To: mart, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart