[ENH] OWLinePlot: Move from prototypes to core

[VesnaT]

Issue

Needs merge of #3432

Description of changes

Includes

• Code changes
• Tests
• Documentation

[ajdapretnar]

I'll do the docs! <3

[janezd]

Fetch me, rebase to move-graph-gui-to-widget and force push; tests should pass then. Your PR will have an additional commit (mine, from #3432), but it will disappear when #3432 is merged (perhaps you'll have to rebase again to get rid of it, or maybe not even that -- I forgot).

[codecov]

Codecov Report

Merging #3440 into master will increase coverage by 0.15%.
The diff coverage is 97.2%.

@@            Coverage Diff             @@
##           master    #3440      +/-   ##
==========================================
+ Coverage   83.27%   83.42%   +0.15%     
==========================================
  Files         365      367       +2     
  Lines       64293    65045     +752     
==========================================
+ Hits        53540    54267     +727     
- Misses      10753    10778      +25

[codecov]

Codecov Report

Merging #3440 into master will increase coverage by 0.12%.
The diff coverage is 97.78%.

@@            Coverage Diff             @@
##           master    #3440      +/-   ##
==========================================
+ Coverage    83.3%   83.42%   +0.12%     
==========================================
  Files         365      367       +2     
  Lines       64409    65067     +658     
==========================================
+ Hits        53654    54284     +630     
- Misses      10755    10783      +28

[janezd]

I assume @lanzagar already knows this code and will take care of the PR? I can volunteer if needed, but would prefer not to, if @lanzagar can do it.

[lanzagar]

I have used the widget and looked at some recent changes, but have not really gone over the whole code layout/structure and wouldn't say I know it. I can give it a semi-quick look sometime next week, otherwise I will be relying on the fact that it has been in prototypes for a long time and seems to be working well.
If anyone else has some time, it would be a plus if they give the code a look as well. 2 quick looks should almost sum to a whole? :)

[janezd]

These are mostly local comments and nitpicking. I haven't found any bugs, but I won't pretend I understand the entire logic of the widget.

[janezd]

self.__groups = None, I suppose. Don't leave any removed items in __groups.

[VesnaT]

Is that really necessary? self.__groups is set to [] right after.

[janezd]

I saw it's set to [] in the method that calls this method. But I'd still prefer to have it here; what if someday we call this method from another place? As a matter of principle I prefer that methods that remove items from graph also remove them from lists.

Compromise: put self.__groups = [] into this method. But if you do this, you should also set it to [] in __init__ and clear, and change all if self.__groups is not None into if self.__groups.

If you dislike both suggestions, ignore them. Or consider them in further development. :)

[VesnaT]

Ok, I like the compromise.

[janezd]

I quite dislike this. It just hides argument names without any benefit. Why not

group.set_visible_error(self.show_error)
group.set_visible_mean(self.show_mean)
group.set_visible_range(self.show_range)
group.set_visible_profiles(self.show_profiles)

One line shorter and four lines cleaner.

[VesnaT]

This does not work correctly (and I should add a test).
Open the widget -> check only mean, close the widget -> open it again -> only mean is checked, but range is also shown.
Two of the set_visible_ methods need two arguments..

[janezd]

I see. But still, give them two arguments then...

[VesnaT]

This one, I would leave it as it is.

[ajdapretnar]

I have the documentation ready, I was just unable to push it on Friday. Git experts welcome for advice.

[ajdapretnar]

Oh well, the docs are here: #3461
Feel free to merge it after this one.

[janezd]

@lanzagar, I guess I won't go deeper into this widget. Unless @VesnaT plans to implement the two suggestions that remain open (setting self.__groups to None or [], and not passing a dict to set_visible_...`), you can merge.