In the files in src/display there are many instances of this
Inkscape::DrawingContext::Save save(dc);
First thing, I don't understand the syntax. What does the lower case "save" in that line indicate? The only places "::Save save(" appear in the source code are instances of this line.
Secondly, it acts strangely. Moving it around breaks graphics for some reason that escapes me. For instance this:
Inkscape::DrawingContext::Save save(dc); //PROBLEM LINE dc.transform(aff); double thickness = decorateItem(dc, aff, phase_length); dc.setLineWidth(thickness); dc.strokePreserve(); dc.newPath();
results in mangled graphics (text decoration drawn as if with a very thick line, but in what appears to be the correct location), whereas moving the first two lines above into
double DrawingText::decorateItem(DrawingContext &dc, Geom::Affine const &aff, double phase_length)
as the first two lines of that method results in normal graphics (text decoration drawn with the expected line thickness). That is:
line1 line2 call
is broken but
call (inside called function) line1 line2
works. Moving "dc.setlinewidth()" from the position shown above into decorateItem() as the line before its return does not make any difference in either of these cases. decorateItem() just defines a cairo path with moveto/lineto on _ct in dc, it doesn't touch anything else in the dc.
Using the debugger and stepping through showed that execution of the problem line passes through this:
DrawingContext::Save::Save(DrawingContext &dc) : _dc(&dc) { _dc->save(); }
in drawing-context.cpp. _dc->save() just does
cairo_save(_ct);
I have never seen anything quite like this when using cairo directly, that is, calling cairo_save() and cairo_restore() has never resulted in mysteriousness of this level. And that seems to be mostly what the DrawingContext does - it keeps track of (private)
cairo_t *_ct;
I would have thought that passing dc by reference should have made the position of the problem line before the call, or just within the called function, a moot point, but apparently not. This is so even though when the address of _ct was checked with dc.raw(), before and after the problem line, it didn't change. Whether or not the problem line was before or inside decorateItem().
Can somebody please explain what is happening here?
Thanks,
David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech
2014/1/24 mathog <mathog@...1176...>:
In the files in src/display there are many instances of this
Inkscape::DrawingContext::Save save(dc);First thing, I don't understand the syntax. What does the lower case "save" in that line indicate? The only places "::Save save(" appear in the source code are instances of this line.
This is a RAII idiom object. The constructor calls cairo_save, while its destructor calls cairo_restore. The line essentially means, "call cairo_save now, and call cairo_restore at the end of this scope." https://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization
If you move this line, some style properties and the drawing transform are not restored to their default after drawing the item, infecting all items which are drawn after it.
Regards, Krzysztof
On Jan 23, 2014, at 4:34 PM, mathog wrote:
In the files in src/display there are many instances of this
Inkscape::DrawingContext::Save save(dc);
First thing, I don't understand the syntax. What does the lower case "save" in that line indicate? The only places "::Save save(" appear in the source code are instances of this line.
Well... the first problem is that the class has a bad name. That is complicating things a bit. The 'Save' class has a verb for a name, whereas we should use nouns for names of classes. Looking into the sources, it seems to be an attempt to merely wrap a call to save paired with a later call to restore. So... just a simple attempt to tune the name would give us "Saver" or perhaps "ContextSaver" instead of "Save".
Inkscape::DrawingContext::ContextSaver saver(dc); dc.transform(aff); double thickness = decorateItem(dc, aff, phase_length); dc.setLineWidth(thickness); dc.strokePreserve(); dc.newPath();
Then whenever the 'saver' instance goes out of scope, either by exiting a closing '}' brace or by an exception being thrown, the call to cairo_restore will be invoked.
However... as we ponder that explanation, a potentially better name presents itself. Could this make the code clearer at the point of use?
{ Inkscape::DrawingContext::ContextRestorer contextRestorer(dc); dc.transform(aff); double thickness = decorateItem(dc, aff, phase_length); dc.setLineWidth(thickness); dc.strokePreserve(); dc.newPath(); }
Then by its name we are given a clue that the *reason* for the class to be around is to restore things. That we first have to save them becomes a secondary implementation detail, where as the "restore" aspect is exposed as the primary use.
On 28-Feb-2014 14:32, Jon Cruz wrote:
On Jan 23, 2014, at 4:34 PM, mathog wrote:
Inkscape::DrawingContext::Save save(dc);
Inkscape::DrawingContext::ContextSaver saver(dc);
Yes, that is less confusing than the original.
That said, the construct is maintenance programmer hostile, since it is hiding something important for not much in return. This appears to be OOP anathema, because the second action must be explicitly coded, but for me the code would be much clearer it it just did the explicit:
cairo_save(ct); ... cairo_restore(ct);
I understand (now) what Inkscape::DrawingContext::Save does, but it took some time to figure it out. Conversely, the cairo save/restore pair is fundamental, and that pair is commonly used in many other places in Inkscape (and hundreds of other programs). If it must be done with the existing methodology, the construct should say clearly what it does:
Inkscape::DrawingContext::CairoContextSaver saved_cairo_context(dc);
Unfortunately this section of Inkscape writes to Cairo for the screen (DrawingContext) in a way that is incompatible with the way it writes to Cairo for file output (CairoRenderContext). These classes aren't derived from each other in any simple way, so reusing the text decoration bits is nontrivial. In fact, the whole text rendering pipeline is sort of, half way, sideways, quasi- replicated in the two cases. Perhaps it's just me, but it seems awfully ironic when the purpose of OOP is to encourage code reuse, that what we have here is a case where wrapping the underlying library (Cairo) in two different and incompatible classes, has discouraged code reuse for the most basic of purposes - writing text!
Regards,
David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech
On Feb 28, 2014, at 3:22 PM, mathog wrote:
On 28-Feb-2014 14:32, Jon Cruz wrote:
On Jan 23, 2014, at 4:34 PM, mathog wrote:
Inkscape::DrawingContext::Save save(dc);
Inkscape::DrawingContext::ContextSaver saver(dc);Yes, that is less confusing than the original.
That said, the construct is maintenance programmer hostile, since it is hiding something important for not much in return. This appears to be OOP anathema, because the second action must be explicitly coded, but for me the code would be much clearer it it just did the explicit:
cairo_save(ct); ... cairo_restore(ct);
Except that is C and not C++. Also, that does not help in the cases where an exception or early return (something I dislike) is encountered before the explicit cairo_restore(ct) call is made.
I think it is just that exceptions and RAII are common C++ approaches that one working in C++ needs to learn and recognize. That said, the names involved can definitely help out in order to signal more explicitly when RAII is in use.
To help with people unfamiliar with the RAII paradigm, I'll add some more explicit and visible info to our wiki, etc.
I understand (now) what Inkscape::DrawingContext::Save does, but it took some time to figure it out. Conversely, the cairo save/restore pair is fundamental, and that pair is commonly used in many other places in Inkscape (and hundreds of other programs). If it must be done with the existing methodology, the construct should say clearly what it does:
Inkscape::DrawingContext::CairoContextSaversaved_cairo_context(dc);
Definitely something to keep in mind as we clean things up. In this case it looks like the code itself is good, and that it only needed more explicit naming to make its intent clear.
participants (4)
-
Jon Cruz -
Krzysztof Kosiński -
Martin Owens -
mathog