Re: Review Request 121831: libksysguard: process.h: encapsulate private fields

[Gregor Mi]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/
---

(Updated Feb. 15, 2015, 4:35 p.m.)


Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.


Repository: libksysguard


Description (updated)
---

This is a follow-up to https://git.reviewboard.kde.org/r/121717/:

In process.h there are several public fields which easily trigger BIC changes. 
This RR introduces a d-ptr.

(In a separate commit I would add the .reviewboardrc file)


Diffs
-

  processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 
  processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 
  processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 
  processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 
  processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 
  processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 
  processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d 
  processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 
  processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 
  processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 
  processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 
  tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 
  .reviewboardrc PRE-CREATION 
  CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 
  processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a 
  processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab 

Diff: https://git.reviewboard.kde.org/r/121831/diff/


Testing
---

Compiles and runs. Data is still shown; no visible error. Unit tests succeed.


Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate private fields

[Gregor Mi]

> On Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote:
> > processcore/processes_linux_p.cpp, line 171
> > 
> >
> > Don't you change behavior here?
> > 
> > Before, we just wrote into the varialbes.
> > 
> > Now, we use the setters, which also sets 'changes |= Process::Gids;'
> > 
> > Is that maybe an issue? I myself don't know the code well enough to see 
> > this here.
> 
> Gregor Mi wrote:
> True. Thanks for noticing. The changes will be read in 
> ProcessModelPrivate::processChanged when a change is signaled by 
> KSysGuard::Process::processChanged. Then GUI model updates are triggered. So 
> probably the worst that can happen is one additional GUI update when 
> processChanged is emitted.
> 
> Gregor Mi wrote:
> I did some analysis. There is no visible change in behaviour ksysguard.
> 
> ProcessesLocal::Private::readProcStatus where the change in discussion 
> was made (setter method with change detection instead of direct assignment).
> 
> is called by ProcessesLocal::updateProcessInfo in the same file:
> 
> ```
> bool ProcessesLocal::updateProcessInfo( long pid, Process *process)
> {
> bool success = true;
> QString dir = "/proc/" + QString::number(pid) + '/';
> if(!d->readProcStat(dir, process)) success = false;
> if(!d->readProcStatus(dir, process)) success = false;  <
>...
> ```
> 
> Then there is
> 
> ```
> bool Processes::updateProcess( Process *ps, long ppid)
> ...
> ...
> 
> bool success = updateProcessInfo(ps); <
> emit processChanged(ps, false);  <---
> 
> return success;
> }
> ```
> 
> ```
> bool Processes::updateOrAddProcess( long pid)
> ...
> ...
> Process *ps = d->mProcesses.value(pid);
> if(!ps)
> return addProcess(pid, ppid);
> else
> return updateProcess(ps, ppid); <-
> }
> ```
> 
> which is called e.g. every second.
> 
> 
> The emitted processChanged signal is connected here:
> 
> ```
> connect( mProcesses, SIGNAL(processChanged(KSysGuard::Process*,bool)), 
> this, SLOT(processChanged(KSysGuard::Process*,bool)));
> ```
> 
> void ProcessModelPrivate::processChanged(KSysGuard::Process *process, 
> bool onlyTotalCpu):
> 
> Each change on a field that was recored in 
> ProcessesLocal::Private::readProcStatus will emit dataChanged signal, e.g.:
> 
> ```
> if(process->changes() & KSysGuard::Process::Uids) {
> totalUpdated++;
> QModelIndex index = q->createIndex(row, 
> ProcessModel::HeadingUser, process);
> emit q->dataChanged(index, index);
> }
> ```
> 
> The dataChanged signal is from QAbstractItemModel. So as far as I can see 
> the worst thing could happen that the GUI is updated at more places than 
> needed.
> 
> When I start ksysguard the processChanged method is called about 300 
> times (1 time for each process in the list) in the update interval (every 
> second).
> 
> I tried to update everything at once and circument the dataChanged of 
> individual items with
> ```
> QModelIndex index1 = q->createIndex(row, 0, process);
> QModelIndex index2 = q->createIndex(row, q->columnCount() - 1, 
> process);
> emit q->dataChanged(index1, index2);
> return;
> ```
> which had not visible effect.
> 
> So what now? Leave the setters and have maybe a performance penalty? 
> Remove the setters again and use references getters? Change the Process API 
> otherwise?

any new input?


- Gregor


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/#review74463
---


On Feb. 15, 2015, 4:35 p.m., Gregor Mi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121831/
> ---
> 
> (Updated Feb. 15, 2015, 4:35 p.m.)
> 
> 
> Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John 
> Tapsell.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> This is a follow-up to https://git.reviewboard.kde.org/r/121717/:
> 
> In process.h there are several public fields which easily trigger BIC 
> changes. This RR introduces a d-ptr.
> 
> (In a separate commit I would add the .reviewboardrc file)
> 
> 
> Diffs
> -
> 
>   processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 
>   processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 
>   pro

Re: Review Request 121361: DeviceAutomounter Settings ui texts are misleading, if not plain wrong.

[Frank Schütte]

> On Dez. 8, 2014, 2:15 nachm., Sebastian Kügler wrote:
> > solid-device-automounter/kcm/DeviceAutomounterKCM.ui, line 45
> > 
> >
> > Well, with this change, the whatsthis and I suppose the function of 
> > this checkbox does the exact opposite of its name.
> > 
> > automountUnknownDevices now means "only automount know devices".
> 
> Thomas Lübking wrote:
> "automountUnknownDevices" is now labeled "Automatically mount unknown 
> devices" and hinted "all attached devices will be automatically mounted[, 
> "otherwise only remembered devices will be"]
> 
> 
> 
> Sounds correct to me, but the automount GUI sucks.
> 
> You've to enable automounting to get a list of four items:
> - known only
> - "all" (does that invoke the "known only" rule?) on login
> - on plugin
> - an override list
> 
> What you indeed have is "on plugin" and "on login", multiplied by the 
> "seen only" rule.
> And then there's an override list, with a lot of unchecked devices what 
> seems to suggest the above rules doesn't actually apply to most things.
> 
> What there should be are two checkboxes:
> [ ] Automount removable media when attached
> [ ] Automount removable media when logging in
> 
> each of them activating a "Device override" group which allows to 
> controlthe override list as well as a third checkbox
> 
> [ ] Do not automount media that has not been mounted before
> 
> And the following override list probably needs to be some sort of 
> tristate - defaulting to no override, what could be done by a leading 
> checkbox column which activates the override for this device itfp.
> 
> Christoph Feck wrote:
> Frank, are you able to propose an updated patch with the above comments 
> addressed?
> 
> Frank Schütte wrote:
> What you suggest seems right to me. Your naming is fine with me. Does "is 
> now labeled" mean, this string is fixed and already in the code, so my patch 
> is useless and I should close this review request ?
> 
> For the rest I am not able to modify the GUI.
> Can I find out, in what version the corrected strings will appear?

Christoph,
sorry I am not able to propose an updated patch. I was just able to fix the 
strings.


- Frank


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121361/#review71555
---


On Dez. 5, 2014, 7:06 nachm., Frank Schütte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121361/
> ---
> 
> (Updated Dez. 5, 2014, 7:06 nachm.)
> 
> 
> Review request for kdelibs, Solid, Christoph Feck, and Helio Castro.
> 
> 
> Bugs: 243046 and 261376
> http://bugs.kde.org/show_bug.cgi?id=243046
> http://bugs.kde.org/show_bug.cgi?id=261376
> 
> 
> Repository: kde-runtime
> 
> 
> Description
> ---
> 
> automounterrc has four settings:
> [General]
> AutomountEnabled=true
> AutomountOnLogin=false
> AutomountOnPlugin=false
> AutomountUnknownDevices=true
> 
> The ui text for AutomountUnknownDevices says the opposite of its 
> functionality. This is repaired by the patch. Login/Plugin enable/disable 
> overrides. I tried to clarify this a little bit.
> 
> 
> Diffs
> -
> 
>   solid-device-automounter/kcm/DeviceAutomounterKCM.ui 3827e95 
> 
> Diff: https://git.reviewboard.kde.org/r/121361/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Frank Schütte
> 
>

Re: Review Request 121361: DeviceAutomounter Settings ui texts are misleading, if not plain wrong.

[Frank Schütte]

> On Dez. 8, 2014, 2:15 nachm., Sebastian Kügler wrote:
> > The patch just fixes two occurrences of confusion, but leaves others. (See 
> > inline for one example.)
> > 
> > It should probably be an approach that fixes the confusion once and for 
> > all. With the naming wrong in the code, bugs are bound to creep in again at 
> > a later point.

No, as I mentioned before, I am not able to propose an updated patch.


> On Dez. 8, 2014, 2:15 nachm., Sebastian Kügler wrote:
> > solid-device-automounter/kcm/DeviceAutomounterKCM.ui, line 45
> > 
> >
> > Well, with this change, the whatsthis and I suppose the function of 
> > this checkbox does the exact opposite of its name.
> > 
> > automountUnknownDevices now means "only automount know devices".
> 
> Thomas Lübking wrote:
> "automountUnknownDevices" is now labeled "Automatically mount unknown 
> devices" and hinted "all attached devices will be automatically mounted[, 
> "otherwise only remembered devices will be"]
> 
> 
> 
> Sounds correct to me, but the automount GUI sucks.
> 
> You've to enable automounting to get a list of four items:
> - known only
> - "all" (does that invoke the "known only" rule?) on login
> - on plugin
> - an override list
> 
> What you indeed have is "on plugin" and "on login", multiplied by the 
> "seen only" rule.
> And then there's an override list, with a lot of unchecked devices what 
> seems to suggest the above rules doesn't actually apply to most things.
> 
> What there should be are two checkboxes:
> [ ] Automount removable media when attached
> [ ] Automount removable media when logging in
> 
> each of them activating a "Device override" group which allows to 
> controlthe override list as well as a third checkbox
> 
> [ ] Do not automount media that has not been mounted before
> 
> And the following override list probably needs to be some sort of 
> tristate - defaulting to no override, what could be done by a leading 
> checkbox column which activates the override for this device itfp.
> 
> Christoph Feck wrote:
> Frank, are you able to propose an updated patch with the above comments 
> addressed?

What you suggest seems right to me. Your naming is fine with me. Does "is now 
labeled" mean, this string is fixed and already in the code, so my patch is 
useless and I should close this review request ?

For the rest I am not able to modify the GUI.
Can I find out, in what version the corrected strings will appear?


- Frank


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121361/#review71555
---


On Dez. 5, 2014, 7:06 nachm., Frank Schütte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121361/
> ---
> 
> (Updated Dez. 5, 2014, 7:06 nachm.)
> 
> 
> Review request for kdelibs, Solid, Christoph Feck, and Helio Castro.
> 
> 
> Bugs: 243046 and 261376
> http://bugs.kde.org/show_bug.cgi?id=243046
> http://bugs.kde.org/show_bug.cgi?id=261376
> 
> 
> Repository: kde-runtime
> 
> 
> Description
> ---
> 
> automounterrc has four settings:
> [General]
> AutomountEnabled=true
> AutomountOnLogin=false
> AutomountOnPlugin=false
> AutomountUnknownDevices=true
> 
> The ui text for AutomountUnknownDevices says the opposite of its 
> functionality. This is repaired by the patch. Login/Plugin enable/disable 
> overrides. I tried to clarify this a little bit.
> 
> 
> Diffs
> -
> 
>   solid-device-automounter/kcm/DeviceAutomounterKCM.ui 3827e95 
> 
> Diff: https://git.reviewboard.kde.org/r/121361/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Frank Schütte
> 
>

Review Request 122573: kio-5.7.0 is required

[Heiko Becker]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122573/
---

Review request for KDE Base Apps.


Repository: kde-baseapps


Description
---

dda83138f9180ca1d54925f78d75ddcf2ca3ec49 introduced the usage of
KFileCopyToMenu. This class was added to kio between 5.6.0 and
5.7.0.


Diffs
-

  CMakeLists.txt 1eda6db 

Diff: https://git.reviewboard.kde.org/r/122573/diff/


Testing
---

cmake .. && make


Thanks,

Heiko Becker