Port KmPlot away from deprecated KStatusBar and kdelibs4support at all
ClosedPublic

Authored by yurchor on Oct 11 2018, 4:02 PM.

Details

Summary

Dependency on the deprecated libraries is removed (I hope). Includes in the affected files are sorted alphabetically.

Test Plan
  • Create some plot (e.g. y=x^2 in the Cartesian coordinates).
  • Move the mouse pointer into the plot area and left-click on the curve.
  • Move the pointer to x=0.
  • Make sure that all labels on the status bar are working again (function name does not work in the unpatched version).
  • Try to export the image, save the plot file, add a parameter and animate the plot, whatever you want... All should work as expected.

Diff Detail

Repository
R334 KmPlot
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
yurchor created this revision.Oct 11 2018, 4:02 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptOct 11 2018, 4:02 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
yurchor requested review of this revision.Oct 11 2018, 4:02 PM
cfeck added a subscriber: cfeck.Oct 11 2018, 5:09 PM
cfeck added inline comments.
CMakeLists.txt
21

Where is IconThemes needed?

kmplot/kmplot.cpp
259

Please some space around operators, they need some air to breathe :)

yurchor marked an inline comment as done.Oct 11 2018, 5:20 PM
yurchor added inline comments.
CMakeLists.txt
21

#include <KIconLoader> (transformed from #include <kiconloader.h>). It compiles and runs without it. I removed the dependency in the updated version of the patch.

yurchor updated this revision to Diff 43408.Oct 11 2018, 5:21 PM
yurchor marked an inline comment as done.

Remove KIconTheme from the dependencies.

yurchor updated this revision to Diff 43410.Oct 11 2018, 5:25 PM
yurchor marked an inline comment as done.

Add spaces.

cfeck added inline comments.Oct 11 2018, 5:37 PM
kmplot/kmplot.cpp
268

Did you actually test this assert? QList indexes start with 0, so if I understand it correctly, it would 'crash' with debugging enabled.

yurchor updated this revision to Diff 43411.Oct 11 2018, 5:40 PM
yurchor marked an inline comment as done.

Remove faulty assert.

cfeck added a comment.Oct 11 2018, 6:11 PM

Checking the previous statusbar code, the id's have a range of 1 ... 4, so there should be 4 sections at list index 0 ... 3, accessed by at(id - 1). The current patch adds 5 sections, and label at index 0 is never used.

I have not tested the patch, but the code to keep the status bar items at fixed sizes was removed, and I fear that hovering over the graphs makes the items all jump around due to sizes changing. Correct?

Checking the previous statusbar code, the id's have a range of 1 ... 4, so there should be 4 sections at list index 0 ... 3, accessed by at(id - 1). The current patch adds 5 sections, and label at index 0 is never used.

I have not tested the patch, but the code to keep the status bar items at fixed sizes was removed, and I fear that hovering over the graphs makes the items all jump around due to sizes changing. Correct?

Yes, it is correct. But there is no jumping. First labels are coordinates, they do not change much in size (I can record a screencast if you like to confirm). The other two labels are jumping in the same manner as in the unpatched version. The original behavior can be easily mimicked by adding several lines of ugly code to asses the width of 16 characters and a condition.

Yes, one label is empty all the time. The code is simpler, I guess that's what we call "the progress" in the new version. Should it be changed to mimic the original by shifting index by one in all the files to remove this extra label?

yurchor updated this revision to Diff 43418.Oct 11 2018, 7:40 PM

Recreate the original KmPlot spacing and number of labels.

yurchor updated this revision to Diff 43530.Oct 13 2018, 9:36 AM

Recreate alignment of the status bar labels to be identical with the unpatched version.

Before the patch:

After the patch:

cfeck accepted this revision.Oct 17 2018, 10:49 PM
cfeck added inline comments.
kmplot/kmplot.cpp
259

Now that you have changed the enum, I would prefer for (int i = 0; i < SectionCount; ++i) and add a SectionCount last item in the enum.

This revision is now accepted and ready to land.Oct 17 2018, 10:49 PM
yurchor marked an inline comment as done.Oct 18 2018, 4:37 AM
yurchor updated this revision to Diff 43836.Oct 18 2018, 4:54 AM

Make enum public, add SectionCount to it, left shift the loop counter by one.

This revision was automatically updated to reflect the committed changes.