Opened 7 months ago

Closed 6 months ago

Last modified 6 months ago

#15968 closed enhancement (fixed)

Allow embedding bitmaps in SVG files generated by wxSVGFileDC

Reported by: iwbnwif Owned by:
Priority: low Milestone:
Component: GUI-all Version:
Keywords: work-needed Cc:
Blocked By: Blocking:
Patch: yes

Description

The current (3.0) implementation of wxSVGFileDC does not embed bitmaps but instead saves them to the same directory as the SVG file it creates and then creates links.

The attached patch provides the option of embedding bitmaps as well as retaining the default behaviour of linking them.

The change has been implemented by creating a 'pluggable' handler for bitmaps. The handler is implemented as simple class derived from a new abstract base class wxSVGFileDCBitmapHandler.

Implementations of the handler class have one method - ProcessBitmap - which includes a choice of linking bitmaps or embedding them.

Two new specialisations of wxSVGFileDC have been added:

1) SetBitmapHandler - deletes the old handler and installs a new one
2) SetEmbedBitmaps - sets a flag to instruct the class whether to embed bitmaps or not

A simple default handler (wxSVGFileDCDefaultHandler) is provided that either embeds PNG files or links them. Linked files currently have the hard coded filename image'n'.png where n is the next available filename in the SVG file's directory. This is a slight regression on the previous implementation where the image filename was prefixed by the SVG filename. The reason for this shortcoming is that (currently) the handler does not have knowledge of the SVG filename.

-- Note: please bear in mind that I am relatively new to wxWidgets!! --

Other things to consider / questions:

  • Is it worth adding a getter to the m_embed_bmps flag?
  • Is there anyway of getting the filename from the wxFileOutputStream object?
  • Please review the way in which I have accessed the specialisations of wxSVGFileDCImpl from wxSVGFileDC
  • Does the handler need to be exported?

Attachments (4)

dcsvg.patch download (8.0 KB) - added by iwbnwif 7 months ago.
dcsvg.2.patch download (6.0 KB) - added by iwbnwif 7 months ago.
interim update
dcsvg.3.patch download (7.7 KB) - added by iwbnwif 6 months ago.
dcsvg.4.patch download (11.8 KB) - added by iwbnwif 6 months ago.

Download all attachments as: .zip

Change History (16)

Changed 7 months ago by iwbnwif

comment:1 Changed 7 months ago by vadz

  • Component changed from base to GUI-all
  • Keywords work-needed added
  • Priority changed from normal to low
  • Status changed from new to confirmed
  • Summary changed from wxSVGFileDC does not embed bitmaps to Allow embedding bitmaps in SVG files generated by wxSVGFileDC

Thanks for the patch!

The main issue I have with it is that it seems that we somehow misunderstood each other on the subject of SetEmbedBitmaps(). For me it was just a convenient helper built on top of the "bitmap handler" API. So ProcessBitmap() shouldn't take bool embed argument, it's the handler itself that decides whether to embed or not. IOW I'd like to replace if(embed)/else structure with two different implementations of the same virtual method in different classes. Otherwise it doesn't make much sense to have this handler at all, we could just add this if to the existing code.

I'd also the virtual method to be more specific than ProcessBitmap() as the current patch duplicates the code for generating the tags in the two if branches when the only thing really different between them is the value of xlink:href attribute. So I'd rather have virtual wxString BuildBitmapHref(const wxBitmap& bmp) which would either create a file and return the name of the file or return the "data:..." string.

Would it be possible for you to update the patch to do it like this?

To answer your questions:

  1. I don't think it makes sense adding a getter for the flag with the approach of using arbitrary handlers.
  2. You can use wxFFileOutputStream::GetFile()->GetName() for this.
  3. Using casts is ugly but there is unfortunately no better way. You could add a helper function to hide them though, e.g. wxSVGFileDCImpl* GetSVGImpl() const.
  4. The handler class should be exported to allow deriving user-defined handlers from it, otherwise this wouldn't link when using a DLL/shared library.

Some general remarks about the patch:

  1. Please make your patches from the top level wxWidgets directory, this makes them simpler to apply automatically.
  2. I'm not sure to understand why should wx/base64.h come after wx/mstream.h? AFAIK there are no dependencies between these headers.
  3. wxSVGFileDC.h doesn't exist...
  4. SetBitmapHandler() doesn't need to be virtual, the other methods are because they're inherited from the base class (and overridden), but this one is not.
  5. The final version must include the changes to the documentation in interface/wx/dcsvg.h. But maybe it's better to stabilize the API before documenting it...

Thanks again!

comment:2 follow-up: Changed 7 months ago by iwbnwif

Sorry, we didn't misunderstand each other - you are right about what we agreed.

The reason I did it this way is because if someone installs their own handler, what should SetEmbedBitmaps() do? For example should it reinstall one of the two 'default' handlers depending on the bool? I couldn't work it out so I though the best thing would be to do it this way!!

Similarly for ProcessBitmap(). I was imagining the case where someone installed their wonderful new handler and it did much more than just BuildBitmapHref().

Thank you very much for your answers and general remarks. I will try to make the next patch better!

BTW I don't want to make putting this patch in place harder work for you than just coding it yourself! I am keen to contribute but also don't want to waste too much of your time.

comment:3 in reply to: ↑ 2 Changed 7 months ago by vadz

Replying to iwbnwif:

Sorry, we didn't misunderstand each other - you are right about what we agreed.

The reason I did it this way is because if someone installs their own handler, what should SetEmbedBitmaps() do? For example should it reinstall one of the two 'default' handlers depending on the bool?

Good question. I'd say that this is a good argument for not providing it at all actually... Let's start with just the "raw" handlers and if it really proves to be too burdensome to use (I doubt it though), we can always return to this later.

Thank you very much for your answers and general remarks. I will try to make the next patch better!

Great, thanks in advance!

BTW I don't want to make putting this patch in place harder work for you than just coding it yourself! I am keen to contribute but also don't want to waste too much of your time.

I hope it will be amortized by your other patches in the future :-) Anyhow, while your patch had a couple of small problems, it really wasn't bad at all for the first try, thanks a lot for your efforts!

Changed 7 months ago by iwbnwif

interim update

comment:4 Changed 7 months ago by iwbnwif

I have uploaded an interim update which addresses some of the things we discussed but I am trying to set up a build and test environment for creating patches which is why things are delayed.

One problem I am having is that I am using ./configure which is modifying tiffconf.h and tif_config.h. Therefore svn diff insists on putting those changes in as well if I run it from the top level directory.

comment:5 Changed 7 months ago by vadz

Hmm, this patch seems to be missing the changes to the headers.

Don't use ./configure: create a separate build directory, which may, but doesn't have to, be a subdirectory and use /full/path/to/configure instead. This will keep your source directory pristine. And will allow you to just rm -rf $build_dir if you want to do a full rebuild.

comment:6 Changed 6 months ago by iwbnwif

vadz, thank you again for your time.

So here is a new patch that (I hope) takes into account your advice and comments.

The only thing that I haven't implemented yet is the change to the function name ProcessBitmap because as mentioned earlier I feel that is what the function needs to do. Please let me know if you think this is still wrong.

Once this is settled I will have a go at producing the documentation. Cheers!

Changed 6 months ago by iwbnwif

comment:7 Changed 6 months ago by iwbnwif

Here is a quick stub that will test the patch

wxBitmap bmp (100, 100, 32);

// draw something on a bitmap
wxMemoryDC dc (bmp);
dc.SetPen(*wxCYAN_PEN);
dc.SetBrush(*wxRED_BRUSH);
dc.SetBackground(*wxBLUE);
dc.Clear();
dc.DrawRectangle(5, 5, 90, 90);
dc.SetPen(*wxRED_PEN);
dc.SetBrush(*wxCYAN_BRUSH);
dc.DrawEllipse(10, 10, 80, 80);
dc.SelectObject(wxNullBitmap); // finish the bitmap

// create a svg with the bitmap embedded
wxSVGFileDC svgDCEmbed ("test_svg_embed.svg", 200, 200); 

svgDCEmbed.SetBitmapHandler(new wxSVGFileDCEmbedBitmapHandler());
svgDCEmbed.DrawBitmap(bmp, 50, 50);

// create a svg with the bitmap embedded
wxSVGFileDC svgDCLink ("test_svg_link.svg", 200, 200); 

svgDCLink.DrawBitmap(bmp, 50, 50);

comment:8 Changed 6 months ago by vadz

This looks mostly fine, the only thing I'd change API-wise would be to pass just wxOutputStream to ProcessBitmap(), it shouldn't matter whether it's a wxFileOutputStream or something else and this will give us more flexibility in the future (e.g. we may want to write SVG directly to the network or whatever).

At the implementation level there is a bit of duplication between the two Process() versions, but I don't know if it's worth adding a separate function in the base class just to format the image size... I'd probably still do it, e.g. add (protected) MakeImageTag(const wxString& url) or something like this to the base class and call it from both versions. But it's a minor point.

Another minor point is that we could slightly optimize this by delaying the creation of the handler until it's really needed -- like this we wouldn't create it unnecessarily in the ctor if SetBitmapHandler() is going to be called right after it anyhow.

But, again, this is pretty minor and the patch could be applied even in its current form, we just need documentation for the new functionality, please update interface/wx/dcsvg.h and it should be good to go.

Thanks!

Changed 6 months ago by iwbnwif

comment:9 Changed 6 months ago by iwbnwif

Replying to vadz:

This looks mostly fine, the only thing I'd change API-wise would be to pass just wxOutputStream > to ProcessBitmap()

Okay, done.

At the implementation level there is a bit of duplication between the two Process() versions, but I don't know if it's worth adding a separate function in the base class just to format the image size... I'd probably still do it, e.g. add (protected) MakeImageTag(const wxString& url) or something like this to the base class and call it from both versions. But it's a minor point.

I looked into this but in the end after I tidied up the code in the two Process() functions I think it will only save one or two lines. So for the minute I haven't implemented it.

Another minor point is that we could slightly optimize this by delaying the creation of the handler until it's really needed -- like this we wouldn't create it unnecessarily in the ctor if SetBitmapHandler() is going to be called right after it anyhow.

Good point, the handler object is now set to NULL on initialization and only instantiated the first time a bitmap is written (or if the user installs a new handler).

please update interface/wx/dcsvg.h and it should be good to go.

Hmmm, I am afraid I haven't used doxygen that much. Anyway I have had a try, hopefully it is more or less okay.

Once again thanks for all your patience.

comment:10 follow-up: Changed 6 months ago by vadz

Thanks! This looks good to me, so I'm committing this with some minor changes:

  1. Rename some classes: sorry for doing it at the last moment but reading the code one final time I realized that the class names were really too long to be convenient to use, so I've abbreviated them and also replaced rather mysterious "Link" with "File" which is IMHO more clear.
  2. Use reference, not pointer, in ProcessBitmap(). As this argument can never be NULL, there is no reason to use pointer here.
  3. Add more comments to explain a casual reader of the headers what is going on here.
  4. Doxygen docs were almost perfect, thanks, I just added @since tags.
  5. Some style fixes: wrap long lines, remove hard TABs, ...

To be totally perfect, the patch would also include a unit test (which is now easier to write as we could just check that the generated SVG includes the expected "data:..." part) and also update the sample with the new functionality but this patch is already very good, especially for your first attempt. Thanks a lot!

comment:11 Changed 6 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [75981]) Allow customizing bitmap handling in wxSVGFileDC.

Provide a built-in alternative for using external files for the bitmaps in
SVG: allow embedding them inside the SVG itself using "data:" URI.

And also allow to define custom handlers to make the behaviour even more
flexible.

Closes #15968.

comment:12 in reply to: ↑ 10 Changed 6 months ago by iwbnwif

  1. Rename some classes: sorry for doing it at the last moment but reading the code one final time I realized that the class names were really too long to be convenient to use, so I've abbreviated them and also replaced rather mysterious "Link" with "File" which is IMHO more clear.

No problems, I will try to get used to the style.

  1. Use reference, not pointer, in ProcessBitmap(). As this argument can never be NULL, there is no reason to use pointer here.

Good point!

  1. Some style fixes: wrap long lines, remove hard TABs, ...

Damn, I thought I had changed all the tabs to spaces.

To be totally perfect, the patch would also include a unit test (which is now easier to write as we could just check that the generated SVG includes the expected "data:..." part) and also update the sample with the new functionality but this patch is already very good, especially for your first attempt. Thanks a lot!

Thank you very much, its been fun and educational!

Note: See TracTickets for help on using tickets.