fix Diagram::PaintDiagram visibility

[felix-salfelder]

this fixes one of the problems addressed in #370

[in3otd]

um, this is practically a revert of c9fcce6#diff-12763b2bbd1d04ec7aa8054d6f637f86 and so brings back this issue

[felix-salfelder]

perhaps its best to revert the full commit then. i was wondering wtf this split into paintThis and paintThat is trying to achieve.

[felix-salfelder]

perhaps its best to revert the full commit then.

i tried. that doesn't seem easy, conflicts with lots of other stuff.

it seems easiest to fix the interface, and fix #522 afterwards. unless anybody can explain #523.
#522 looks like a boundary box problem, and does not need weird hacks.

[guitorri]

Hum. It seams that the direct calls to paintMarkers was needed to sort out the printing, update it maybe? Could be an issue of bounding box, but could also related with current implementation of the rendering loop. See postPaintEvent. Every drawStuff() is added to a list and rendered in order. Maybe, if things are out of order they could be hidden or clipped out during print (even if shown properly on the screen). The above paintMarker hack changes the order on postPaintEvent.

The following is not going to help, but I must mention.
Direct calls to paint() are not allowed with the QGraphics port. Items do the paint() by themselves.
Similarly ViewPainter is not needed. Just want to mention that all this code has been or is being refactored by me. I will publish the branch once I see the light on the other end. It is mostly working, but I want to keep the noise low and work on my own pace.
What I want to say is, if possible, to hold your horses on matters touching the paint() stuff.

[felix-salfelder]

paintMarkers does not exist in the element interface. so it does not make sense to call it from the outside. markers are attached to graphs, graphs live within diagrams etcpp.

rendering loop

cant fix the rendering loop before fixing the basics.

Items do the paint() by themselves.

this sounds promising, thanks for mentioning. in particular that means that we will end up with exactly one "paint" method. close to what i want to achieve with this PR, no?

[in3otd]

IIRC, in origin it was a wrong boundary box issue but while fixing that the ability to print a Diagram with or without Markers was added (depending whether the Markers were selected with the Diagram or not) and this led to splitting the Paint() method.

[felix-salfelder]

so that splitting is only needed internally, right?

(i did not touch it, and i do not have a problem with that)

[in3otd]

well, it depends on the exact meaning of "internally"; the Schematic needs to be able to select printing/showing a Diagram with or without Markers.

[felix-salfelder]

if somebody wants to hide markers, then (s)he should implement diagram::(un)hideMarkers.

not part of this PR.

[felix-salfelder]

or... if markers need to be top-level objects in the schematic, no problem. it does not require paint hacks either.

[in3otd]

I've added a couple of commits that fixes the issue with printing and undoes the artificial splitting of paint() done some time ago.

[felix-salfelder]

marker::isHidden looks weird to me, particularly in public.

why not

marker::paint(){
     if(hidden()){
          //nop
     }else{
            [paint stuff]
     }
 }

(in addition, flag access must go through function calls)

[in3otd]

yep, it's better if Marker::paint() decides whether to paint the marker or not.

Regarding the flag access, you mean we should have something like

void hide() {hidden = true;};
void unHide() {hidden = false;};

in Marker()? Seems mostly a style preference to me but, well, this is your PR 😁
(similarly, in the if construct above I'll tend to just return early instead of putting most of the code in the else branch).

[in3otd]

code rearranged to let Marker::paint() decide whether to paint the marker or not.
(Edit: fixed typo)

[felix-salfelder]

done in refactor branch