D27061: replace samba module with data that works

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R102:f1768f7b3ab9: replace samba module with data that works 
(authored by sitter).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D27061?vs=75170&id=75272#toc

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27061?vs=75170&id=75272

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

AFFECTED FILES
  Modules/samba/CMakeLists.txt
  Modules/samba/autotests/CMakeLists.txt
  Modules/samba/autotests/ksambasharemodeltest.cpp
  Modules/samba/autotests/smbmountmodeltest.cpp
  Modules/samba/kcmsambaimports.cpp
  Modules/samba/kcmsambaimports.h
  Modules/samba/kcmsambalog.cpp
  Modules/samba/kcmsambalog.h
  Modules/samba/kcmsambastatistics.cpp
  Modules/samba/kcmsambastatistics.h
  Modules/samba/ksambasharemodel.cpp
  Modules/samba/ksambasharemodel.h
  Modules/samba/ksmbstatus.cpp
  Modules/samba/ksmbstatus.h
  Modules/samba/main.cpp
  Modules/samba/smbmountmodel.cpp
  Modules/samba/smbmountmodel.h

To: sitter, #localization, #plasma, #vdg, davidedmundson
Cc: jriddell, davidedmundson, ltoscano, yurchor, ngraham, alexde, plasma-devel, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27061: replace samba module with data that works

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  I'm happy with the model.
  
  As for 5.18. I would normally be super against, but if it really didn't work 
before, then there's no point blindly following the rules if the user 
experience would be ultimately worse. Your call

REPOSITORY
  R102 KInfoCenter

BRANCH
  smb

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

To: sitter, #localization, #plasma, #vdg, davidedmundson
Cc: jriddell, davidedmundson, ltoscano, yurchor, ngraham, alexde, plasma-devel, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27061: replace samba module with data that works

[Harald Sitter]

sitter updated this revision to Diff 75170.
sitter added a comment.


  move from qmap to straight up qlist as values() within the map wouldn't 
necessarily have the expected order. instead find_by udi on add/remove and rely 
on index within the list the rest of the time
  
  additional qa: have 2 mounts, mount/umount randomly

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27061?vs=74992&id=75170

BRANCH
  smb

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

AFFECTED FILES
  LICENSES/GPL-2.0-only.txt
  LICENSES/GPL-2.0-or-later.txt
  LICENSES/GPL-3.0-only.txt
  LICENSES/LicenseRef-KDE-Accepted-GPL.txt
  LICENSES/MIT.txt
  Modules/samba/CMakeLists.txt
  Modules/samba/autotests/CMakeLists.txt
  Modules/samba/autotests/ksambasharemodeltest.cpp
  Modules/samba/autotests/smbmountmodeltest.cpp
  Modules/samba/kcmsambaimports.cpp
  Modules/samba/kcmsambaimports.h
  Modules/samba/kcmsambalog.cpp
  Modules/samba/kcmsambalog.h
  Modules/samba/kcmsambastatistics.cpp
  Modules/samba/kcmsambastatistics.h
  Modules/samba/ksambasharemodel.cpp
  Modules/samba/ksambasharemodel.h
  Modules/samba/ksmbstatus.cpp
  Modules/samba/ksmbstatus.h
  Modules/samba/main.cpp
  Modules/samba/smbmountmodel.cpp
  Modules/samba/smbmountmodel.h

To: sitter, #localization, #plasma, #vdg
Cc: jriddell, davidedmundson, ltoscano, yurchor, ngraham, alexde, plasma-devel, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27061: replace samba module with data that works

[Jonathan Riddell]

jriddell added a comment.


  As release mangler for Plasma I'd normally point out this is a breach of 
feature freeze, but it seems to have the support of important Plasma people so 
great go ahead when ready.

REPOSITORY
  R102 KInfoCenter

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

To: sitter, #localization, #plasma, #vdg
Cc: jriddell, davidedmundson, ltoscano, yurchor, ngraham, alexde, plasma-devel, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27061: replace samba module with data that works

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> smbmountmodel.cpp:73
> +case ColumnRole::Share:
> +return 
> devices().at(index.row()).as()->url();
> +case ColumnRole::Path:

This looks dangerous.

You always add row indexes to the end, but you're taking an index of 
map.values() for the source.

Qmap::values retains order within entires, but the whole list is not ordered by 
global insertion time.

i.e If you call addDevice it could insert in the middle of what .values returns 
even though from a model pov you've said it's at the end. This will break a UI.

REPOSITORY
  R102 KInfoCenter

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

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

D27061: replace samba module with data that works

[Luigi Toscano]

ltoscano added a comment.


  As no one else complained, and given the general brokeness of the current 
code, I'd say: please go for it. 5.18.0 is already tagged, there are ~11 days 
before 5.18.1 (planned for Tue 11th) and ~18 before 5.18.2, so we will probably 
catch up.
  
  +1 from the localization point of view (I can't comment on the rest of the 
code).

REPOSITORY
  R102 KInfoCenter

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

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

D27061: replace samba module with data that works

[Nathaniel Graham]

ngraham added a comment.


  #localization  folks, can we 
get an approval or rejection for this?

REPOSITORY
  R102 KInfoCenter

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

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

D27061: replace samba module with data that works

[Harald Sitter]

sitter updated this revision to Diff 74992.
sitter added a comment.


  typo--

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27061?vs=74764&id=74992

BRANCH
  smb

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

AFFECTED FILES
  LICENSES/GPL-2.0-only.txt
  LICENSES/GPL-2.0-or-later.txt
  LICENSES/GPL-3.0-only.txt
  LICENSES/LicenseRef-KDE-Accepted-GPL.txt
  LICENSES/MIT.txt
  Modules/samba/CMakeLists.txt
  Modules/samba/autotests/CMakeLists.txt
  Modules/samba/autotests/ksambasharemodeltest.cpp
  Modules/samba/autotests/smbmountmodeltest.cpp
  Modules/samba/kcmsambaimports.cpp
  Modules/samba/kcmsambaimports.h
  Modules/samba/kcmsambalog.cpp
  Modules/samba/kcmsambalog.h
  Modules/samba/kcmsambastatistics.cpp
  Modules/samba/kcmsambastatistics.h
  Modules/samba/ksambasharemodel.cpp
  Modules/samba/ksambasharemodel.h
  Modules/samba/ksmbstatus.cpp
  Modules/samba/ksmbstatus.h
  Modules/samba/main.cpp
  Modules/samba/smbmountmodel.cpp
  Modules/samba/smbmountmodel.h

To: sitter, #localization, #plasma, #vdg
Cc: yurchor, ngraham, alexde, 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

D27061: replace samba module with data that works

[Yuri Chornoivan]

yurchor added a comment.


  Thanks in advance for fixing these minor typos.

INLINE COMMENTS

> main.cpp:65
> +auto title = new KTitleWidget(this);
> +// hackily remove ampersand to not break i18n too too much in 5.18
> +// TODO: replace i18n'd string to one without quick access marker

Typo: too too -> too

> smbmountmodel.cpp:53
> +case ColumnRole::Accessible:
> +return i18nc("@title:column whether a sama share is accessible 
> locally (i.e. mounted)", "Accessible");
> +case ColumnRole::ColumnCount:

Typo: sama -> samba

REPOSITORY
  R102 KInfoCenter

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

To: sitter, #localization, #plasma, #vdg
Cc: yurchor, ngraham, alexde, 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

D27061: replace samba module with data that works

[Harald Sitter]

sitter added a comment.


  I was trying to recycle as many strings as possible. If the localizers are 
fine with us throwing caution to the wind for those two strings as well then 
that's fine with me naturally, it would also allow me to get rid of a naughty 
hack I had to pull to retain the strings.
  If not, we can improve the strings in master for 5.19.

REPOSITORY
  R102 KInfoCenter

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

To: sitter, #localization, #plasma, #vdg
Cc: ngraham, alexde, 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

D27061: replace samba module with data that works

[Nathaniel Graham]

ngraham added reviewers: Localization, Plasma, VDG.
ngraham added a comment.


  Very nice. Looks much better and seems to work quite well.
  
  While we're already breaking proposing breaking the string freeze, instead of 
"Exports" and "Imports", how about "Exported Shares" and "Mounted shares" or 
"Accessed shares"? Needs #localization 
 approval for that of course.

REPOSITORY
  R102 KInfoCenter

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

To: sitter, #localization, #plasma, #vdg
Cc: ngraham, alexde, 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

D27061: replace samba module with data that works

[Harald Sitter]

sitter updated this revision to Diff 74764.
sitter added a comment.


  translate accessible bool to icons + resize its column. looks better and 
avoids yet more new strings

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27061?vs=74762&id=74764

BRANCH
  smb

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

AFFECTED FILES
  LICENSES/GPL-2.0-only.txt
  LICENSES/GPL-2.0-or-later.txt
  LICENSES/GPL-3.0-only.txt
  LICENSES/LicenseRef-KDE-Accepted-GPL.txt
  LICENSES/MIT.txt
  Modules/samba/CMakeLists.txt
  Modules/samba/autotests/CMakeLists.txt
  Modules/samba/autotests/ksambasharemodeltest.cpp
  Modules/samba/autotests/smbmountmodeltest.cpp
  Modules/samba/kcmsambaimports.cpp
  Modules/samba/kcmsambaimports.h
  Modules/samba/kcmsambalog.cpp
  Modules/samba/kcmsambalog.h
  Modules/samba/kcmsambastatistics.cpp
  Modules/samba/kcmsambastatistics.h
  Modules/samba/ksambasharemodel.cpp
  Modules/samba/ksambasharemodel.h
  Modules/samba/ksmbstatus.cpp
  Modules/samba/ksmbstatus.h
  Modules/samba/main.cpp
  Modules/samba/smbmountmodel.cpp
  Modules/samba/smbmountmodel.h

To: sitter
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

D27061: replace samba module with data that works

[Harald Sitter]

sitter updated this revision to Diff 74762.
sitter added a comment.


  unbreak model actually

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27061?vs=74761&id=74762

BRANCH
  smb

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

AFFECTED FILES
  LICENSES/GPL-2.0-only.txt
  LICENSES/GPL-2.0-or-later.txt
  LICENSES/GPL-3.0-only.txt
  LICENSES/LicenseRef-KDE-Accepted-GPL.txt
  LICENSES/MIT.txt
  Modules/samba/CMakeLists.txt
  Modules/samba/autotests/CMakeLists.txt
  Modules/samba/autotests/ksambasharemodeltest.cpp
  Modules/samba/autotests/smbmountmodeltest.cpp
  Modules/samba/kcmsambaimports.cpp
  Modules/samba/kcmsambaimports.h
  Modules/samba/kcmsambalog.cpp
  Modules/samba/kcmsambalog.h
  Modules/samba/kcmsambastatistics.cpp
  Modules/samba/kcmsambastatistics.h
  Modules/samba/ksambasharemodel.cpp
  Modules/samba/ksambasharemodel.h
  Modules/samba/ksmbstatus.cpp
  Modules/samba/ksmbstatus.h
  Modules/samba/main.cpp
  Modules/samba/smbmountmodel.cpp
  Modules/samba/smbmountmodel.h

To: sitter
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

D27061: replace samba module with data that works

[Harald Sitter]

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

REVISION SUMMARY
  ... and doesn't require lots of maintenance!
  this targets 5.18 but is kind of unfortunate because it needs a bunch of
  new strings as no existing strings provide what is needed here.
  
  the previous module was super broken in various ways.
  in the interest of maintainability I've thrown everything out and replaced
  it with 2 core features which require only modeling code on the KInfoCenter
  end and provide actually (possibly) useful functionality to the design
  personas of plasma.
  
  there is now a single page which contains two table views:
  
  a) Exports: this is a simple table of shares "exported" from the host. the
  
data for this comes from KSambaShare in KIOCore. this is the same API
used by dolphin to create new shares, so the data will be consistent
and the API needs maintaining anyway
  
  b) Imports: simple table of shares "imported" (i.e. mounted) onto the host.
  
the data for this comes from solid
  
  both are backed by models, with an eye towards a future port to qml (out of
  scope since I want this fixed for 5.18)
  
  all previous functionality was removed, in part because it was doing CLI
  parsing, some of the parsing was broken, some of the CLI tools couldn't
  even run as !root, log parsing could use incorrect paths on existing users,
  log parsing has nothing to parse with samba defaults, log parsing didn't
  implement per-host log file support (current default in samba), log parsing
  didn't correctly implement per-user-config-log-file support.
  in short: there was not a single feature that worked properly.
  
  BUG: 411433
  BUG: 374141
  BUG: 325951

TEST PLAN
  exports
  ===
  
  - nothing when nothing is exported
  - changing exports via dolphin is immediately reflected in the kcm
  - export data is valid
  
  imports
  ===
  
  - nothing when nothing is mounted
  - (un)mounting a cifs updates the view immediately
  - data is valid

REPOSITORY
  R102 KInfoCenter

BRANCH
  smb

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

AFFECTED FILES
  LICENSES/GPL-2.0-only.txt
  LICENSES/GPL-2.0-or-later.txt
  LICENSES/GPL-3.0-only.txt
  LICENSES/LicenseRef-KDE-Accepted-GPL.txt
  LICENSES/MIT.txt
  Modules/samba/CMakeLists.txt
  Modules/samba/autotests/CMakeLists.txt
  Modules/samba/autotests/ksambasharemodeltest.cpp
  Modules/samba/autotests/smbmountmodeltest.cpp
  Modules/samba/kcmsambaimports.cpp
  Modules/samba/kcmsambaimports.h
  Modules/samba/kcmsambalog.cpp
  Modules/samba/kcmsambalog.h
  Modules/samba/kcmsambastatistics.cpp
  Modules/samba/kcmsambastatistics.h
  Modules/samba/ksambasharemodel.cpp
  Modules/samba/ksambasharemodel.h
  Modules/samba/ksmbstatus.cpp
  Modules/samba/ksmbstatus.h
  Modules/samba/main.cpp
  Modules/samba/smbmountmodel.cpp
  Modules/samba/smbmountmodel.h

To: sitter
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