OpenRaster and OpenDocument enhancement

[Alan Horkan]

TL:DR; Summary: I wrote a new version of the file import export code OraFormat.cs

OpenRaster is layered image format, and has a minimal specification.
OpenDocument is lots of different file formats, and an enormous specification.

OpenRaster is loosely inspired by OpenDocument, it can and it SHOULD make use of more of the OpenDocument file format.
To better prove this point I wanted to be able to show code. Pinta seemed like a a suitable choice (I liked the license and it gave me a reason to try programming in C#). I have extended the OpenRaster code and added OpenDocument draw compatibility, so that if you rename the .ora output files to .odg they will open in LibreOffice/OpenOffice Draw.
There is more that could be done, but changes to other parts of Pinta besides OraFormat.cs would probably be needed and I wanted to limit the scope of my changes. Next would probably be to write a Properties Dialog and allow users to add Metadata (something OpenRaster does not specify but an ideal opportunity to copy what OpenDocument already recommends).

My changes are against Pinta 1.4 which might make it more difficult to merge. My code is missing some changes from OraFormat.cs where Layer was changed to UserLayer, I think it was related to the Text tool updates but I don't think I missed anything else.

Besides the code related to adding OpenDocument support there are other notable changes:
Rewrote and replaced XmlTextWriter with XmlWriter
"Starting with the .NET Framework 2.0, we recommend that you use the System.Xml.XmlWriter class instead."
http://msdn.microsoft.com/en-us/library/system.xml.xmltextwriter%28v=vs.110%29.aspx

Added support for OpenRaster Layer Visibility, import and export
(Previously Pinta incorrectly set opacity="0" when a layer was set to hidden.)
http://freedesktop.org/wiki/Specifications/OpenRaster/Draft/LayersStack/

The code is heavily indented, and sparsely commented.
Code review appreciated, particularly if I've written anything in a non C# style.

[Robert Nordan (rpvn)]

HI, it's great to get new contributions! Before we can have a closer look at it, it would be nice if you could prepare a GtiHub pull request as described in this document: https://github.com/PintaProject/Pinta/blob/master/patch-guidelines.md . That will make it a lot easier for us to review the code.

But even without looking at it, I'm thinking that sounds like a pretty substantial rewrite of the ora provider, can we be sure that we don't introduce any issues with ora files, especially backwards compatability with existing Pinta-made ora? (Also, having to rename to odg files afterwards is not ideal.) Because of that I'm a bit sceptical to merging this into the Pinta core, but have no fear, we have an even better solution! This looks like an ideal candidate for a file format add-in!

See this guide: https://github.com/PintaProject/Pinta/wiki/Writing-a-File-Format-Addin

I imagine you probably wouldn't have to do a lot of work to package this as a new addin, and then you could support .odg files directly, as well as add in the metadata dialogue you were talking about. We'd love to see more addins now that we have a functional addin infrastructure!

[Alan Horkan (horkana)]

> But even without looking at it,

Please do look at it.
Drop it in place of your existing OraFormat and using any diff tool see for yourself
(Meld is a particularly nice graphical tool http://meldmerge.org/ )

> I'm thinking that sounds like a pretty substantial rewrite of the ora provider,

It actually far simpler than it sounds. I hope if you look at it you will see that.

> can we be sure that we don't introduce any issues with ora files,

I've tested it a lot to get to work. The OpenRaster part is relatively uncomplicated, and the complexity is not so much in the code it is learning the OpenDocument standard and testing to see what the was the least markup I could get away with.

(There is an ugly hack conversion of pixels to centimeters, I would not have thought was necessary until I tested it against Office, but that is on the OpenDocument side unrelated to the OpenRaster.)

> especially backwards compatability with existing Pinta-made ora?

Old Open Raster files made in Pinta are not a problem.

(As I mentioned before the old code treated a hidden layer the same as a layer having opacity=0 which was a strange broken behaviour anyway. I noticed this mistake after testing the sample OpenRaster documents. It did not make sense to try and work around that old broken behaviour. https://gitorious.org/openraster/openraster-example-files )

There are features of OpenRaster that Pinta does not yet implement. Some are relatively simple such as: locked layers; layer selected; the requirement of a mergedimage.png. Other are more complicated, such as the goal that OpenRaster should be able to pass through unknown information -- such as vector objects created by MyPaint .ora files. These missing features are an interoperability problem.

The OpenRaster specification is a minimal specification, that simplicity is good, it makes the format easy to understand and implement but it is limited. This uses existing standards to extend it, adding more alongside the existing OpenRaster information, using OpenDocument as a superset of OpenRaster.

At the risk of oversimplifying OpenDocument MUST include a manifest listing the contents of the archive, and MUST include content.xml and this code adds those things to the OpenRaster file. (Making it both OpenRaster and OpenDocument compatible as a result.) Following these extra requirements of OpenDocument benefits OpenRaster making it more interoperable, and more compatible with a greater set of tools.

> (Also, having to rename to odg files afterwards is not ideal.)

That is only a test to show compatibility. My intention was to extend the OpenRaster format, and recommend to developers that OpenRaster can and should reuse as much as possible of OpenDocument standards first. It was not my intention to implement OpenDocument Draw support.**

> Because of that I'm a bit sceptical to merging this into the Pinta core, but have no fear, we have an even better solution! This looks like an ideal candidate for a file format add-in!

The key reason I wrote this was to show that how OpenDocument overlaps and can be a superset of OpenRaster.
If someone was to redo this as an add-in, I would hope at least som...

[Robert Nordan (rpvn)]

You make a compelling case for including it into the core, especially when it comes to fixing broken behaviour and improving interoperability. I can't sign off on this alone though, Cameron needs to give his input here.

In the meantime, I must insist for all our sakes on getting the formalities right, as having a reviewable pull request on GitHub makes it easy for us to give proper feedback and for you to update it in response to that. There's no time to learn git like the present!

After a quick look at the code I have three initial pieces of feedback:
- You have to make it work with the current master of Pinta if it's to be mergable.
- The global variable with version can be replaced with PintaCore.ApplicationVersion
- The pixel/centimetre conversion can be replaced with a GDK lookup http://stackoverflow.com/questions/417384/get-dpi-settings-via-gtk

Love the effort you've put into it so far, let's see if we can't reel this one all the way in!

[Alan Horkan (horkana)]

Thanks for the advice on the version code. Looking closer I can see now I missed this because I was working off of version 1.4, and master does include a cleaner more elegant solution.
https://github.com/PintaProject/Pinta/blob/master/Pinta/Dialogs/AboutPintaTabPage.cs

If I wasn't focused on the file format I'd probably be trying to fix Pinta to include the command line arguments --help and --version that gnu always recommends, and it helps to have a properly abstracted version number there too. https://www.gnu.org/prep/standards/html_node/Command_002dLine-Interfaces.html

[Cameron White (cameronwhite91)]

A couple months ago I added "--help" and "--version" arguments to Pinta. What errors were you getting when trying to build 1.5 and master?

[Alan Horkan (horkana)]

Thanks Cameron, good to know the standards are implemented now.

Tried it again. Only getting warnings:
Warning CS0108: 'Pinta.Effects.PolarInversionEffect.Data' ... [etc]
Warning MSB3247: Found conflicts between different versions of the same dependent assembly.
Field 'Pinta.MacInterop.DescType.Value' is never assigned to, and will always have its default value

The problem may have been my unfamiliarity with C# and SharpDevelop.

Grabbed a copy of Pinta in Zip from Github. (Down to the same few warnings.)
Merged my OraFormat.cs to include the latest changes.
Fixed the version call as suggested.

Attaching new version ...

[Robert Nordan (rpvn)]

That's great! But I still must insist on you using a pull request to submit your work, as it is much easier for us to do the review and integration that way. (For example with the Travis CI buildbot making sure the changes build, proper tracing of changes and easier line by line review.) Since you're on Windows you can try GitHub for Windows, which makes things pretty easy: https://windows.github.com/ . Once you have it set up and a branch created, you can pretty much just drop in your changed OraFormat.cs in the folder and git will pick up all the differences, presenting a nice diff for us to review once you commit and make a pull request.

[Alan Horkan (horkana)]

There are more things in the code I want to on, but I don't have a lot of time to work on it, weekends maybe.

> insist on you using a pull request to submit your work,

I'll figure that out too eventually, but it has taken lots of time to get to this point.
(Setting up a working build environment if you haven't done it before takes time. It's only easy in hindsight.)

Before I put more work into it though, is it likely to be accepted?
Is there a timeline for the version 1.6 release I should be aware of?

While this has been a lot of work for me, at the end I still only expect it to be a line item on the changelog:
> OpenRaster format improvements.

I'll try to do more in a few days.

[Alan Horkan (horkana)]

Pull request
https://github.com/PintaProject/Pinta/pull/79

Next putting together a simplistic Properties dialog.

[Alan Horkan (horkana)]

Two steps forward one step back.
Made some progress but having difficulties getting things pushed to github.

Knocked together an ugly Properties dialog (labels and text entries basically) and made a few other changes but they are incomplete, still a few pieces need to be glued into place. Maybe I'll be be able to figure it out myself but I may need some advice on how to organise the backend logic needed for metadata, but that should all make more sense when I can push the code to github and submit an updated pull request.

[Alan Horkan (horkana)]

I've done a chunk more work, unfortunately I lost and had to redo some of it.
I hope you can review some of it soon.

I've implemented an ugly Properties dialog and there is a backend code to support it (following the model use by Layers, this is not necessarily a good design but it seems to work) and OpenRaster exporter OraFormat.cs writes the metadata out to meta.xml part of the file.

The code to read the Properties/Metadata back in still needs to be written, I hope to get it done in the next week or two.

[Alan Horkan (horkana)]

if my discussing the code on IRC would be more efficient than going back and forth through the github review page or this bug report please let me know and maybe we can arrange a suitable time.

[Robert Nordan (rpvn)]

Sorry, I was just landed with a major project at work, so will have limited
time to do open source stuff for another couple of weeks. One question
though: how does the properties dialogue work, does it come up every time
you want to save a new ora or is it more voluntary?
On 2 Nov 2014 21:55, "Alan Horkan" <email address hidden> wrote:

> if my discussing the code on IRC would be more efficient than going back
> and forth through the github review page or this bug report please let
> me know and maybe we can arrange a suitable time.
>
> --
> You received this bug notification because you are a member of Pinta
> Maintainers, which is subscribed to Pinta.
> https://bugs.launchpad.net/bugs/1377566
>
> Title:
> OpenRaster and OpenDocument enhancement
>
> Status in Pinta:
> New
>
> Bug description:
> TL:DR; Summary: I wrote a new version of the file import export code
> OraFormat.cs
>
>
> OpenRaster is layered image format, and has a minimal specification.
> OpenDocument is lots of different file formats, and an enormous
> specification.
>
> OpenRaster is loosely inspired by OpenDocument, it can and it SHOULD
> make use of more of the OpenDocument file format.
> To better prove this point I wanted to be able to show code. Pinta
> seemed like a a suitable choice (I liked the license and it gave me a
> reason to try programming in C#). I have extended the OpenRaster code and
> added OpenDocument draw compatibility, so that if you rename the .ora
> output files to .odg they will open in LibreOffice/OpenOffice Draw.
> There is more that could be done, but changes to other parts of Pinta
> besides OraFormat.cs would probably be needed and I wanted to limit the
> scope of my changes. Next would probably be to write a Properties Dialog
> and allow users to add Metadata (something OpenRaster does not specify but
> an ideal opportunity to copy what OpenDocument already recommends).
>
> My changes are against Pinta 1.4 which might make it more difficult to
> merge. My code is missing some changes from OraFormat.cs where Layer
> was changed to UserLayer, I think it was related to the Text tool
> updates but I don't think I missed anything else.
>
>
> Besides the code related to adding OpenDocument support there are other
> notable changes:
> Rewrote and replaced XmlTextWriter with XmlWriter
> "Starting with the .NET Framework 2.0, we recommend that you use the
> System.Xml.XmlWriter class instead."
>
> http://msdn.microsoft.com/en-us/library/system.xml.xmltextwriter%28v=vs.110%29.aspx
>
> Added support for OpenRaster Layer Visibility, import and export
> (Previously Pinta incorrectly set opacity="0" when a layer was set to
> hidden.)
> http://freedesktop.org/wiki/Specifications/OpenRaster/Draft/LayersStack/
>
>
> The code is heavily indented, and sparsely commented.
> Code review appreciated, particularly if I've written anything in a non
> C# style.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/pinta/+bug/1377566/+subscriptions
>

[Alan Horkan (horkana)]

Thanks for responding. I'm working in small bursts myself, I'm not in any great hurry but I'd hate to miss the next release.

Properties dialog comes up only when requested, much like Openoffice etc.
I don't like to see more than one dialog when saving.
The Properties dialog is in the file menu and looks very much like the first screenshot I posted above.

There are minor flaws that I'm aware of, but my concern is if have made any stupid architectural or design decisions?
For example, maybe changing Document.cs from
public class Document
to
public class Document : ObservableObject
is a terrible decision and I should be making the changes somewhere else. Maybe I should be breaking up my code into shorter methods or abstracting things out in some way.

Some but not all of the flaws: After learning more and using StyleCop I don't expect any further C# style issues. The Properties dialog is functional but not pretty, it is far from looking like the second screenshot. Layer locking is a flag, that flag gets loaded and is preserved on save and there is a checkbox in the Layers dialog allow that flag to be set or unset but users are not actually prevented from editing a layer. (If you actually lock a layer and prevent users from editing, it needs to be very clear what is happening and why and how to turn it off the lock. Most implementations of locking are annoying and confusing.) Some of the diffs are almost useless because I tried to make some style fixes, something as simple as replacing all tabs with 4 spaces makes it difficult to see any other change in the diffs.

[Alan Horkan (horkana)]

Gave this another nudge forward. Some of the metadata is now being imported back. No error handling yet.
Proof of concept shown, more needed to get it to release standard.
https://github.com/PintaProject/Pinta/pull/79

[Alan Horkan (horkana)]

I've done more work on this and I think I'm very close to finished. (Basic error handling turned out to be straightforward enough in hindsight)

At this point I need a little help:

1. I'd like the comments text box in the properties dialog to have scroll a vertical scroll widgets. I've struggled to figure out how to do that (and regret ever writing UI code in GTK, learning how to integrate generated .ui files can't be much worse than this was).

2. I'm happy with my method to convert from pixels to centimetres (and I don't think it is actually important) but since it was one of the first things that you asked me to correct I have tried and failed to figure out how exactly I'm supposed to get the DPI

Aside from that I'm happy with the code, I've improved the OpenRaster support, I've been strict and included a manifest, and I've extended OpenRaster to include metadata in the same way as OpenDocument.

[grofaty (grofaty)]

Alan, you should probably discuss this exciting new feature in Pinta's forum at Google Mailing List: https://groups.google.com/forum/?hl=en#!forum/pinta A lot of users are reading mailing list, but just a few are reading bug tracker.

[Cameron White (cameronwhite91)]

Sorry I haven't been following these discussions much, but I'll look at this after Pinta 1.6 is out

[Alan Horkan (horkana)]

How long before Pinta 1.7? (or Pinta 1.6.x ?)

There were some bugfixes could have been cherrypicked for 1.6 but I wasn't aware of the timeline for the 1.6 release.

One fix in particular (included the first pull request from October) was for opacity and visibility.
Pinta was pretending that hidden layers were the same things as layers with Opacity set to zero.
https://github.com/horkana/Pinta/commit/c83df4e3f6f00c86c4c65d74200c8edc5581fc91

mergedimage.png is important too (also included in the October patch).

[grofaty (grofaty)]

Alan, Pinta 1.6 was released few hours ago, so we are now officially in Pinta 1.7 development cycle.

[Alan Horkan (horkana)]

I know 1.6 was released. I'm disappointed none of my work was included.

That is why I am asking when the next release will be or if there will any bugfix releases, like many other projects do.

Pinta 1.6 could be around for a very long time. I'd be even more disappointed to see a linux distribution ship a long term release without at least cherry-picking some of the bugfixes.

[Cameron White (cameronwhite91)]

The problem with submitting very large pull requests (currently 47 commits and around 1300 new lines of code) is that it's very time-consuming to do a thorough code review, and if the pull request contains some work-in-progress code, any bug fixes won't be immediately merged in (and in particular, with most of the focus for last couple months being on stabilizing Pinta for 1.6, I wasn't looking to add in large changes just before the release). Submitting smaller pull requests makes it considerably easier for me to review and pull in changes.

To answer your question about Pinta 1.7, I'd definitely like the gap between 1.6 and 1.7 to be much shorter than it was for 1.5 -> 16. Something like a few months, probably.

[Alan Horkan (horkana)]

The first pull request only touched one file, and wasn't really so big but had lots of minor flaws as it was the first c# I'd written. Many of the other patches were very small, as I was working on my local repository and giving myself I high level of detail. (I've recently learned I can go back and amend the git history and append the smaller patches together. I'm not yet comfortable managing multiple branches, otherwise I'd have kept more of the changes separate, things like the menu mnemonics and the code indentation.)

If there had been more communication it wouldn't have been so bad, I didn't have enough information.
All of this is increasingly obvious in hindsight, I'm learning or relearning as I go along.

Perhaps if Pinta had an up to date MAINTAINERS file that might make things it clearer and make it easier for new contributors to manage their expectations
i.e. Cameron is the most active (lead) developer, other active developers are ...
... Pinta aims for release every 6-8 months,
... patches MUST be in the form of pull requests*
... things should be discussed on the mailing list before the bug tracker
things like that

* http://pinta-project.com/contribute
I know the contribute page says some of this but it says "you'll want to make your changes available as pull requests on GitHub" and tries to be all friendly but it reads very much like a SHOULD or "please" when it should be clear that it really is a MUST.

I'm sure this all makes sense to you but from the outside it isn't obvious. (Until Cameron posted on March 1, I was still aware of the possibility that my changes might be rejected entirely and I'd have to fork and release my own builds.)

I'm still willing to push this forward and I'm still interested in trying to make other improvements (see bug 1388211, making the changes themselves should be trivial, managing them in a way that you'll be happy is the actual hard part, big chunks small chunks) after we get at least some of this sorted.

Pinta Forge is pretty cool too, I'm less worried about major releases when that is effectively putting out minor releases on a regular basis, so hopefully more people will consider using them.

[grofaty (grofaty)]

Alan, I see you are very passionate about Pinta and I love it. You got to know that Pinta is a pretty small project and mainly runs on one person and few of them assisting. You jumped in with huge changes that are not easy to digest, specially because most of the merges are perfomed by only one great lead maintainer, Cameron, who was very busy fixing bugs and pusing a Pinta 1.6 release. The best approach is to create one small fix and create a pull request that is easy to understand and can be checked if working fine and then merged into project master.

Your interest in Pinta makes me very happy. It looks like you are not just trying to fix one or two annoying bugs, but would like to make more significant changes. In my humble opinion you may be perfect candidate for Google Summer of Code 2015, see: https://groups.google.com/forum/?hl=en#!topic/pinta/2eyE-N4XQ34 - you know you could work on Pinta code, have official mentor, personal e-mail contact of developer to help you through troubles, get advices and not the least and get some money out of it, you know money never hurts.

Andrew was one of very successful GSoC project (in 2013 and 2014), see also: https://groups.google.com/forum/?hl=en#!searchin/pinta/gsoc <--- searches for GSoC search string from Pinta's mailinglist.

If you are interested you have to prepare a list of task you would be working on. See end-users feature requests: https://pinta.uservoice.com/forums/105955-general

If interested please post some info about your interest on: https://groups.google.com/forum/?hl=en#!topic/pinta/2eyE-N4XQ34 and someone from developers can get in touch with you if help is required.

[Alan Horkan (horkana)]

grofaty the summer of code is for students, I'm not a student.

[Alan Horkan (horkana)]

I do not intend to work on this any further. You may do whatever you want with the code.

There are at least two bugs in Pinta that need to be fixed and I would strongly encourage you to take the code to fix these two problems at least:

1) Opacity zero not the same as Visibility False
2) OpenRaster standard requires mergedimage.png (a full sized preview image)

[Cameron White (cameronwhite91)]

I'm planning to look at the open pull requests after 1.7, since this was quite a large one to consider merging right before the release.

[Cameron White (cameronwhite91)]

I've committed the .ora bug fixes in https://github.com/PintaProject/Pinta/commit/632c56607005d57a295c2d942496813873a95745 and https://github.com/PintaProject/Pinta/commit/dcb5c7af2b5c2a18f452431405b54dae8a2c90d7