Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per bug 1377566, OpenRaster format improvements, and OpenDocument standard. #79

Closed
wants to merge 49 commits into from

Conversation

horkana
Copy link

@horkana horkana commented Oct 14, 2014

includes fix for opacity/visibilty;
enhancements: selected layer; mergedimage; OpenDocument compatibility;

…sing OpenDocument standard.

includes fix for opacity/visibilty;
enhancements: selected layer; mergedimage; OpenDocument compatibility;
@robpvn
Copy link
Member

robpvn commented Oct 14, 2014

Great that you putin the effort to make a pull request! I'm trying out your branch as we speak.

@robpvn
Copy link
Member

robpvn commented Oct 14, 2014

I liked being able to open the file in Openoffice after renaming! BUt I see that we can't do it the other way around, becaue LibreOffice odgs don't have a stack.xml.

@horkana
Copy link
Author

horkana commented Oct 14, 2014

I saw that the dominant brace style was
blah
{
}

but it seems like there are inconsistencies,
blah () {
}

public void Import (string fileName, Gtk.Window parent) {

being the first example. Or is there a subtlety I'm missing?

I'm hoping you will say Github or Pinta has something like lint (or some other utility) that will automatically correct all those format issues before merging?

(My screen is small and I prefer not to depend on automatic word wrap so my code might be more heavily indented and have more line breaks than is entirely necessary.)

@horkana horkana closed this Oct 14, 2014
@horkana horkana reopened this Oct 14, 2014
@robpvn
Copy link
Member

robpvn commented Oct 14, 2014

No subtleties, but inconsistencies tend to sneak in. They are of course evil and to be derided for their evil non-consistency. No magic GitHub tool, but fairly quick to fix.

Fair warning: I'm signing off for the night and may or may not have a lot of time left over until next week, so expect me to be inconsistent in my participation. ;-)

@horkana
Copy link
Author

horkana commented Oct 14, 2014

As I said on the bug report it will be mostly weekends for me, but I'll try to keep an eye on this and the bug report.

@horkana
Copy link
Author

horkana commented Oct 15, 2014

I liked being able to open the file in Openoffice after renaming!

I think it is a big win for users at the cost only a few kilobytes of extra XML
(not so nice for developers maybe)

BUt I see that we can't do it the other way around, because LibreOffice odgs don't have a stack.xml.

That is an effect, it is not the cause. LibreOffice ODGs are mostly vector drawings, like SVG.
OpenRaster stack.xml does a lot of the same things as content.xml does in OpenDocument, but OpenRaster so far is little more than a PNG for every layer and a bit of XML to glue it together.
(You could easily do the same and write HTML, SVG, or various other types of XML (eg Pencil2d) as the glue.)

As I said in the bug report the purpose behind this code is to show that OpenRaster can and should do more to overlap with OpenDocument.

Krita already uses ODG for their vector layers, reusing OpenDocument
http://lists.freedesktop.org/archives/create/2014-August/005005.html
OpenDocument used ODG (and a subset of SVG) because there were limitations to SVG, I've always thought this reusing of existing standards was very prudent.

I think doing more with meta.xml is a better example of the kind of easy overlap I want to show works well. There are other ways to do metadata (such as Adobe XMP packets) but there is a benefit to staying within the OpenDocument standard and copying their approach and only very little short term benefit to doing any differently. The downside is figuring out what subset of OpenDocument you actually want to use and writing all the verbose markup.

…sing OpenDocument standard.

includes fix for opacity/visibilty;
enhancements: selected layer; mergedimage; OpenDocument compatibility;
Conflicts:
	Pinta.Core/ImageFormats/OraFormat.cs
…h git: git makes complicated things possible but the easy things are not simple enough.)
Ran StyleCop against the project, resulting in over 10000 warnings. Made some small style fixes to the files I am changing anyway: avoid tabs use spaces instead, be slightly stricter about formatting of braces.
(bringing me back to where I was a few weekends ago before I accidentally deleted my work by misunderstanding git)
…ork.

next connecting it to to the OraFormat exporter, read and write
Make it easier to cleanly relabel custom builds (e.g. previews,  nightly builds, unofficial builds) or forks.
author, title, subject.
no error handling yet
author, title, subject.
no error handling yet
Conflicts:
	Pinta.Core/ImageFormats/OraFormat.cs
History still needs fixing
don't call PintaCore.Workspace.ActiveDocument directly, instead scope things properly and pass the document
trying not to make the Import and Export methods any longer than they already are.
also added some line breaks to try and avoid very wide lines.
Bugs = new Gtk.Action ("Bugs", Catalog.GetString ("File a Bug"), null, "Menu.Help.Bug.png");
Translate = new Gtk.Action ("Translate", Catalog.GetString ("Translate This Application"), null, "Menu.Help.Translate.png");
About = new Gtk.Action ("About", Catalog.GetString ("About"), null, Stock.About);
Website = new Gtk.Action ("Website", string.Format("{0} {1}", PintaCore.ApplicationName, Catalog.GetString ("_Website")), null, "Menu.Help.Website.png");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format string needs to be translatable (string.Format (Catalog.GetString ("{0} _Website"))) since grammar rules can be very different in other languages.

You may want to place a comment (prefixed with "Translators:" so that our automated tools pick it up) on the preceding line to explain to translators that the application name will be substituted there, since the meaning of the string may be confusing otherwise.

in case anyone decides to write support for Flat ODG
(consider it an exercise to force better structure and abstraction of the code)
menu item will be the Application name (Pinta) + followed by the word Website.

(I'd leave out the Application name entirely, but the previous change was intended to be minimal and was merely a distraction from other problems I was trying to figure out at the time.)
Bugs = new Gtk.Action ("Bugs", Catalog.GetString ("File a Bug"), null, "Menu.Help.Bug.png");
Translate = new Gtk.Action ("Translate", Catalog.GetString ("Translate This Application"), null, "Menu.Help.Translate.png");
About = new Gtk.Action ("About", Catalog.GetString ("About"), null, Stock.About);
// Translators: "Application name" (Pinta) + "Website"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the comment for translators.
The format string itself still needs to be made translatable, though - see my previous comment.for what it should look like

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've misunderstood what you were asking for here.

Tried again but and thought I almost had it (something to do with Catalog.GetString marking it as a translatable string but I'm not getting it quite right) but will need to leave it until I've more time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, this isn't well-documented - I've added a wiki page (https://github.com/PintaProject/Pinta/wiki/Translations) about how translations work, which hopefully helps

@horkana
Copy link
Author

horkana commented Mar 3, 2015

I don't expect I'll have time to look at this again before the weekend (definitely not time to rewrite anything, maybe time to read and comment here, and read the code maybe).

Thanks for your comments.

@cameronwhite
Copy link
Member

OK, no problem. I think I've reviewed everything except for the .ora format changes, so I'll try to look at those by the weekend.

Keep the backend so Locked setting gets passed though but do not show it as Pinta doesn't do anything to honour the setting, only passes through the flag/setting.
@aggsol
Copy link

aggsol commented Jul 31, 2019

Is this merged anytime?

@cameronwhite
Copy link
Member

I've merged the .ora bug fixes in dcb5c7a and 632c566

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants