On Wed, 2023-02-22 at 20:49 +0100, Francesco Pretto wrote:
> an update on the PdfPainter situation
Hi,
thanks for looking on this so quickly. I did not make any real code
review, these are just few thoughts, but really only thoughts, it can
stay as you have it now and I'll be fine with it.
- 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().
- 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).
- Unrelated thing to the painter changes, I never liked the source
structure of the PoDoFo, the `base` and the `doc`. It would make
sense if there are two libraries, one for the `base` and then
one for the `doc`, but the sources are split only on the disk,
they construct one library only. 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. What about avoiding this artificial and
unnecessary split on the disk/directory level and place all the files
into a single directory? There's only that 'private' static library,
but that can be avoided, especially if it's linked to a single
target, right?
- 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.
Again, these are only thoughts. Feel free to ignore me. Thank you for
your work on all of this.
Bye,
zyx
_______________________________________________
Podofo-users mailing list
Podofo-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/podofo-users
