On Thu, 23 Feb 2023 at 08:32, zyx <z...@gmx.us> wrote: > - The PdfPainterPath::GetView() sounds weird. When it comes to views, > then it's something like GUI-related for me. The Path has a > `content`, not a view. It returns a string_view, which is something > I do not know. The PdfStringStream has GetString(), not GetView(). >
No big deal, I can rename GetView() to GetString(). In a sense I was thinking that one may not expect a PdfPainter class to have a GetString() method. But if also GetView() can be confusing, and we don't want to name it more specifically, we can be more conservative an have GetString() instead. > - When looking into (for example) DrawRectangle(), I expected to see > a use of a local `path` object, not to call drawing operations > indirectly - it feels like if you are going to change something, > you'll do it in two+ places (in the painter and in the path). > Actually this is something I really never pushed for. Before I had this PdfPainter::Path local object which was really a path being constructed (but not stroke/filled immediately). I ended removing that and introducing the external path construction with PdfPainterPath, so we didn't miss the ability to inspect it before pushing it to the painter, as you requested. PdfPainter::Draw... like functions will really do stroke/fill right after the call, so it doesn't mix well with the concept of "under construction". What I think you are suggesting here is more categorization: you may suggest to have a local object to just, for example, draw closed shapes (eg. a PdfPainter::Shapes). I think categorization may be interesting to consider but if you introduce it the consequence is that everything should obey to that concept, and we may come up a situation where it's hard to preserve. The only local "object" accessibility I introduced, and I think it's good to preserve, is PdfPainter::TextObject. This really helps separate those text functions that have similarities with path construction (MoveTo(), add text in current position...) from the other PdfPainter::Draw... like functions. In this way PdfPainter is really similar in working to an established API (the often mentioned .NET Graphics class), and without missing some of peculiarities of the PDF specification. In conclusion, I think the current design improved a lot after your early thoughts/comments. I also believe there's no need for further categorization. > You changed it to `common`, `main`, > `private`, `staging` and apart of `staging` being really confusing > name (does it have anything to do with "git staging"? I doubt it), > it makes it really hard to find the right place when searching > for the source files. I agree the previous source layout was confusing and also hardly enforceable. Yes, "podofo_private" was added, but it's privately used also by tests and tools, so we are not single target anymore. I believe there's some more rationale for the separation I picked, I explained everything in the SOURCE-LAYOUT.md document[1]. Note: "staging" should remind you of linux kernel staging folder. > - You should consider including podofo_config.h into each .cpp file > as the first include file, even when it looks like it's not needed > at the moment. The config file can contain switches for the other > include files, changing behavior for the compiler. Maybe in the > future, if not now. > I think at moment (and maybe in 0.9.x too) podofo_config.h[2] has not really any definition that is needed to be first in the compilation units. You may have noticed (or not) that all/most compilation units now have this include first: #include <podofo/private/PdfDeclarationsPrivate.h> That's the header where one is supposed to put early defines for compilation units, if needed. > Again, these are only thoughts You're welcome. I think answering these and others help in better clarifying development choices, that may not be always obvious at first glance. Cheers, Francesco [1] https://github.com/podofo/podofo/blob/master/SOURCE-LAYOUT.md#source-directory [2] https://github.com/podofo/podofo/blob/master/src/podofo/podofo_config.h.in _______________________________________________ Podofo-users mailing list Podofo-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/podofo-users
