Discussion:
Review Request 124485: Add missing traffic monitor
Jan Grulich
2015-07-27 10:06:02 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------

Review request for Network Management and Plasma.


Bugs: 268022
http://bugs.kde.org/show_bug.cgi?id=268022


Repository: plasma-nm


Description
-------

See summary.


Diffs
-----

applet/contents/ui/ConnectionItem.qml ff722a7

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


Testing
-------


Thanks,

Jan Grulich
David Edmundson
2015-07-27 10:13:17 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83026
-----------------------------------------------------------


screenshot and add usability?

- David Edmundson
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated July 27, 2015, 10:06 a.m.)
Review request for Network Management and Plasma.
Bugs: 268022
http://bugs.kde.org/show_bug.cgi?id=268022
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
Thanks,
Jan Grulich
Jan Grulich
2015-07-27 10:17:29 UTC
Permalink
Post by Jan Grulich
Post by David Edmundson
screenshot and add usability?
Added screenshot and usability group. I also already talked to Thomas and Jens and they are fine with this change.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83026
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated Čec. 27, 2015, 10:06 dop.)
Review request for Network Management and Plasma.
Bugs: 268022
http://bugs.kde.org/show_bug.cgi?id=268022
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
Thanks,
Jan Grulich
David Edmundson
2015-07-27 10:19:59 UTC
Permalink
Post by Jan Grulich
Post by David Edmundson
screenshot and add usability?
Added screenshot and usability group. I also already talked to Thomas and Jens and they are fine with this change.
ah, that's all I needed to hear :)


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83026
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated July 27, 2015, 10:17 a.m.)
Review request for Network Management, Plasma and KDE Usability.
Bugs: 268022
http://bugs.kde.org/show_bug.cgi?id=268022
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)
Thanks,
Jan Grulich
Jan Grulich
2015-07-27 10:17:39 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------

(Updated Čec. 27, 2015, 10:17 dop.)


Review request for Network Management, Plasma and KDE Usability.


Bugs: 268022
http://bugs.kde.org/show_bug.cgi?id=268022


Repository: plasma-nm


Description
-------

See summary.


Diffs
-----

applet/contents/ui/ConnectionItem.qml ff722a7

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


Testing (updated)
-------

![Traffic monitor](Loading Image...)


Thanks,

Jan Grulich
Heiko Tietze
2015-07-27 10:47:29 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83033
-----------------------------------------------------------

Ship it!


Why are the graphs such unrealistic shaped? Looks like heavy filtering. If Thomas and Jens granted usability I will check the ship it here. But please discuss also the scaling method. When it is adopted to the current speed the actual graph is not really informative; if the y-axis' scaling is fix to the max. bandwidth it might be a problem in case of other bottlenecks. I suggest to (optionally) scale by the max measured value therefore.

- Heiko Tietze
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated Juli 27, 2015, 10:17 vorm.)
Review request for Network Management, Plasma and KDE Usability.
Bugs: 268022
http://bugs.kde.org/show_bug.cgi?id=268022
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)
Thanks,
Jan Grulich
Jan Grulich
2015-07-27 10:53:10 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
Why are the graphs such unrealistic shaped? Looks like heavy filtering. If Thomas and Jens granted usability I will check the ship it here. But please discuss also the scaling method. When it is adopted to the current speed the actual graph is not really informative; if the y-axis' scaling is fix to the max. bandwidth it might be a problem in case of other bottlenecks. I suggest to (optionally) scale by the max measured value therefore.
That's the way how it's implemented in KDeclarative framework, I don't know if it's possible to change it and I don't have knowledge to do it myself. It scales according to the maximal visible value of the graph.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83033
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated Čec. 27, 2015, 10:17 dop.)
Review request for Network Management, Plasma and KDE Usability.
Bugs: 268022
http://bugs.kde.org/show_bug.cgi?id=268022
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)
Thanks,
Jan Grulich
David Edmundson
2015-07-27 11:12:28 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
Why are the graphs such unrealistic shaped? Looks like heavy filtering. If Thomas and Jens granted usability I will check the ship it here. But please discuss also the scaling method. When it is adopted to the current speed the actual graph is not really informative; if the y-axis' scaling is fix to the max. bandwidth it might be a problem in case of other bottlenecks. I suggest to (optionally) scale by the max measured value therefore.
That's the way how it's implemented in KDeclarative framework, I don't know if it's possible to change it and I don't have knowledge to do it myself. It scales according to the maximal visible value of the graph.
there is a property to explcitly set a max for this purpose; but you need automatic scaling here I think as you have no idea what the max speed of the network is.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83033
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated July 27, 2015, 10:55 a.m.)
Review request for Network Management, Plasma and KDE Usability.
Bugs: 333070
http://bugs.kde.org/show_bug.cgi?id=333070
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)
Thanks,
Jan Grulich
Jan Grulich
2015-07-27 10:55:24 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------

(Updated Čec. 27, 2015, 10:55 dop.)


Review request for Network Management, Plasma and KDE Usability.


Bugs: 333070
http://bugs.kde.org/show_bug.cgi?id=333070


Repository: plasma-nm


Description
-------

See summary.


Diffs
-----

applet/contents/ui/ConnectionItem.qml ff722a7

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


Testing
-------

![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)


Thanks,

Jan Grulich
David Edmundson
2015-07-27 11:12:39 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83030
-----------------------------------------------------------



applet/contents/ui/ConnectionItem.qml (line 292)
<https://git.reviewboard.kde.org/r/124485/#comment57315>

I don't get why 5?



applet/contents/ui/ConnectionItem.qml (line 519)
<https://git.reviewboard.kde.org/r/124485/#comment57314>

can you document this?


- David Edmundson
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated July 27, 2015, 10:55 a.m.)
Review request for Network Management, Plasma and KDE Usability.
Bugs: 333070
http://bugs.kde.org/show_bug.cgi?id=333070
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)
Thanks,
Jan Grulich
Jan Grulich
2015-07-27 11:15:39 UTC
Permalink
Post by Jan Grulich
applet/contents/ui/ConnectionItem.qml, line 292
<https://git.reviewboard.kde.org/r/124485/diff/1/?file=388017#file388017line292>
I don't get why 5?
Because there are 5 lines in the plotter.
Post by Jan Grulich
applet/contents/ui/ConnectionItem.qml, line 519
<https://git.reviewboard.kde.org/r/124485/diff/1/?file=388017#file388017line519>
can you document this?
Actually I can't, I just copied this part from the system monitor applet, but it just generates the second color used for upload.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83030
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated Čec. 27, 2015, 10:55 dop.)
Review request for Network Management, Plasma and KDE Usability.
Bugs: 333070
http://bugs.kde.org/show_bug.cgi?id=333070
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)
Thanks,
Jan Grulich
David Edmundson
2015-07-27 11:20:26 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83040
-----------------------------------------------------------

Ship it!


Ship It!

- David Edmundson
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated July 27, 2015, 10:55 a.m.)
Review request for Network Management, Plasma and KDE Usability.
Bugs: 333070
http://bugs.kde.org/show_bug.cgi?id=333070
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)
Thanks,
Jan Grulich
Lamarque Souza
2015-07-27 12:57:27 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83041
-----------------------------------------------------------



applet/contents/ui/ConnectionItem.qml (line 306)
<https://git.reviewboard.kde.org/r/124485/#comment57319>

what's the update rate used here? I think it is still 1 second. The old Plasma NM used to use 2 seconds instead. Increasing the update time can also help with the update overhead.



applet/contents/ui/ConnectionItem.qml (line 346)
<https://git.reviewboard.kde.org/r/124485/#comment57321>

I do not understand why returning when source name is found. Sorry for not looking at DataSource documentation for the answer but my laptop is malfunctioning. I am doing this review from my tablet.



applet/contents/ui/ConnectionItem.qml (line 519)
<https://git.reviewboard.kde.org/r/124485/#comment57318>

so at least write the original file path you copied that from.


- Lamarque Souza
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated July 27, 2015, 10:55 a.m.)
Review request for Network Management, Plasma and KDE Usability.
Bugs: 333070
http://bugs.kde.org/show_bug.cgi?id=333070
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)
Thanks,
Jan Grulich
Jan Grulich
2015-07-27 13:14:06 UTC
Permalink
Post by Jan Grulich
applet/contents/ui/ConnectionItem.qml, line 306
<https://git.reviewboard.kde.org/r/124485/diff/1/?file=388017#file388017line306>
what's the update rate used here? I think it is still 1 second. The old Plasma NM used to use 2 seconds instead. Increasing the update time can also help with the update overhead.
Also 2 seconds.
Post by Jan Grulich
applet/contents/ui/ConnectionItem.qml, line 346
<https://git.reviewboard.kde.org/r/124485/diff/1/?file=388017#file388017line346>
I do not understand why returning when source name is found. Sorry for not looking at DataSource documentation for the answer but my laptop is malfunctioning. I am doing this review from my tablet.
Hmm, I don't remember right now, but I guess it checks whether the sourceName starts with "network/interfaces/DeviceName".
Post by Jan Grulich
applet/contents/ui/ConnectionItem.qml, line 519
<https://git.reviewboard.kde.org/r/124485/diff/1/?file=388017#file388017line519>
so at least write the original file path you copied that from.
Ok, will do that.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/#review83041
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------
(Updated Čec. 27, 2015, 1:08 odp.)
Review request for Network Management, Plasma and KDE Usability.
Bugs: 333070
http://bugs.kde.org/show_bug.cgi?id=333070
Repository: plasma-nm
Description
-------
See summary.
Diffs
-----
applet/contents/ui/ConnectionItem.qml ff722a7
Diff: https://git.reviewboard.kde.org/r/124485/diff/
Testing
-------
![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)
Thanks,
Jan Grulich
Jan Grulich
2015-07-27 13:08:15 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124485/
-----------------------------------------------------------

(Updated July 27, 2015, 1:08 p.m.)


Status
------

This change has been marked as submitted.


Review request for Network Management, Plasma and KDE Usability.


Changes
-------

Submitted with commit e20839806450cd018de2e7a24fb23a54a70fe6da by Jan Grulich to branch master.


Bugs: 333070
http://bugs.kde.org/show_bug.cgi?id=333070


Repository: plasma-nm


Description
-------

See summary.


Diffs
-----

applet/contents/ui/ConnectionItem.qml ff722a7

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


Testing
-------

![Traffic monitor](https://jgrulich.fedorapeople.org/plotter.png)


Thanks,

Jan Grulich
Loading...