replace samba module with data that works
ClosedPublic

Authored by sitter on Jan 31 2020, 12:06 PM.

Details

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
FIXED-IN: 5.18.0

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

Diff Detail

Repository
R102 KInfoCenter
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sitter created this revision.Jan 31 2020, 12:06 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 31 2020, 12:06 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sitter requested review of this revision.Jan 31 2020, 12:06 PM
sitter updated this revision to Diff 74762.Jan 31 2020, 12:11 PM

unbreak model actually

sitter edited the summary of this revision. (Show Details)Jan 31 2020, 12:11 PM
sitter updated this revision to Diff 74764.Jan 31 2020, 12:28 PM

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

sitter edited the summary of this revision. (Show Details)Jan 31 2020, 12:29 PM
alexde added a subscriber: alexde.Jan 31 2020, 12:41 PM
ngraham added a subscriber: ngraham.

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.

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.

ngraham accepted this revision as: VDG.Jan 31 2020, 4:35 PM

Thanks in advance for fixing these minor typos.

Modules/samba/main.cpp
65

Typo: too too -> too

Modules/samba/smbmountmodel.cpp
54

Typo: sama -> samba

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

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).

davidedmundson added inline comments.
Modules/samba/smbmountmodel.cpp
74

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.

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.

sitter updated this revision to Diff 75170.Feb 7 2020, 3:15 PM

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

davidedmundson accepted this revision.Feb 8 2020, 5:06 PM

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

This revision is now accepted and ready to land.Feb 8 2020, 5:06 PM
This revision was automatically updated to reflect the committed changes.