Merge lp://qastaging/~gaspa/a4/fix-605324 into lp://qastaging/a4

I haven't tested it, because I've seen that the code is a bit too dirty and I'd like to see it cleaned up a bit first. :-)

For app.py:

* You have specified the encoding. Why have you done so and, if there's a reason to specify it, why haven't you used UTF-8?

* You are using gtk.Dialog instead of gtk.MessageDialog, why?

* I'd be really better to use the buttons from the stock instead of creating them with `add_button`.

* s/mgs/msg/

* During the import (although currently isn't a time-expensive operation), it'd be better not to destroy the dialog. This will implicitly tell the user "A4 is importing your file, don't touch anything!".

* After "except PresentationError as error" you have changed the error message. There are two issues with this:

  1. PresentationError doesn't mean that the metadata is missing;
  2. the user may not know what the 'metadata' is.

For editor.py:

* The messages placed in PresentationError exceptions will be displayed to the user, so they should be worded with care. Using the 'metadata' word makes the message very unfriendly, in my opinion. I'd prefer something like "The file you are trying to open has been created with a version of A4 newer than this one", maybe adding some instructions on how to proceed.

* You are using the list 'available_versions' to check whether A4 can handle the file or not. Since we support just one version, I think this isn't good and here's why. Suppose that in one of the next releases (e.g. 0.4) we change the metadata format (and therefore its version) and suppose that a user tries to open a 0.1 file with A4 0.4. Now, A4 probably won't be able to use the 0.1 file directly, but will have to upgrade it first. Upgrading means calling some functions before making the file ready to use and probably also means displaying a message to the user. So, although 0.1 will be an available_version, it can't be used directly and should be special-cased. Probably we'll need to place all the supported versions in a dictionary, with (version, upgrade_function) pairs, probably we'll need some more complex methods. What I propose is, for now, to check the version with "== '0.1'" (instead of "in available_vers") and leave the work of handling old metadata for the next releases.

Finally: PEP8, PEP8, PEP8. ;-)

Sorry for being so fussy, but I love to have code always clean, easy to maintain and on high quality.

> * You have specified the encoding. Why have you done so and, if there's a
> reason to specify it, why haven't you used UTF-8?

to be removed. It was here just for some italian strings (already removed)

> * You are using gtk.Dialog instead of gtk.MessageDialog, why?
> * I'd be really better to use the buttons from the stock instead of creating
> them with `add_button`.

I didn't really understand how to use MessageDialog.
waiting for a working snippet ;)

> * You are using the list 'available_versions' to check whether A4 can handle
> the file or not. Since we support just one version, I think this isn't good
> and here's why.
[...big-snip...]

it can be removed, if you want, but I'll redo in a future refactoring.
My guess is to check if it's "a supported version", and then let a function handle the real selection of the version, so I find useful a list of versions.

> Finally: PEP8, PEP8, PEP8. ;-)
> Sorry for being so fussy, but I love to have code always clean, easy to
> maintain and on high quality.

yep, too fussy. :P
We already discussed in pvt, so waiting for a thread from you in ML.

On Thu, 2010-09-02 at 08:54 +0000, Andrea Gasparini wrote:
> > * You are using the list 'available_versions' to check whether A4
> can handle
> > the file or not. Since we support just one version, I think this
> isn't good
> > and here's why.
> [...big-snip...]
>
> it can be removed, if you want, but I'll redo in a future refactoring.
> My guess is to check if it's "a supported version", and then let a
> function handle the real selection of the version, so I find useful a
> list of versions.

Probably, but not necessarily. The problem is that you don't know what
do we need because we currently support just one version. To support old
versions in the future, probably we'll need a list, probably a
dictionary, probably a new module or probably a completely new
infrastructure. Placing the code now, without having a plan or any
valuable data, just makes the current architecture unnecessarily
complicated.

$ ./a4 tests/images/drawing.svg
Traceback (most recent call last):
  File "./a4", line 8, in <module>
    main()
  File "/tmp/fix-605324/a4lib/app.py", line 298, in main
    window.open_file(files[0])
  File "/tmp/fix-605324/a4lib/app.py", line 88, in open_file
    dialog.vbox.pack_start(gtk.Label(msg))
NameError: global name 'msg' is not defined

Tested the following:
- when opening a plain SVG, a dialog asks whether to import, if clicking "yes" it adds metadata correctly;
- if clicking no, it does nothing :-)
- when opening a A4 file with correct version, it works as usual;
- when opening a file with unknown metadata version, it complains and does not open it.

So, unless you want to try out some corner cases, I'd say it does what it should.

Just one question: when opening a plain SVG, wouldn't it be better to save the metadata only after the user clicks "save" instead of modifying the file when *opening* it? What do you think?
Just an idea, it's not blocking, and in case it may be filed as a separate bug.

In my opinion this is something that shouldn't happen. If a user imports a SVG file, probably (s)he'll want to keep the original image as is, without overwriting it. The use case is: "As a user, I want to import a SVG file, so that I can build my presentation on it", so it's not implicit that he wants to trash his original SVG.

Instead, the user should be given the ability to import the image without modifying it and to save it in an another location. That means: the import process shouldn't write to the image (like Andrea G. proposed) and the "Save" button should be disabled, in favor of "Save As".

Ideally, we would use a different extension for presentations (e.g. .a4 instead of .svg) to avoid such confusion.

> In my opinion this is something that shouldn't happen. If a user imports a SVG
> file, probably (s)he'll want to keep the original image as is, without
> overwriting it. The use case is: "As a user, I want to import a SVG file, so
> that I can build my presentation on it", so it's not implicit that he wants to
> trash his original SVG.

Trash?! why trash? At the moment we don't make any modifications that stop users still use their SVG as an svg file. :)
(that's not the case, but) even when we'll have a complete editor, it'll work as a normal SVG editor as well.
So, users will still have their svg images, and their a4 presentation.

> Ideally, we would use a different extension for presentations (e.g. .a4
> instead of .svg) to avoid such confusion.

Well, though the above sentences, I'm not completely opposed to have an our file type.
Anyway, I see this as a different issue than the one addressed by the merge.
Let's open a discussion on the ML, or on a bug report.

IMHO we have to decide just for the issue raise by Andrea G. (Save immediatly our metadata stub, or wait the user clicks on 'save' button?), and let the rest on other report and merge.

On Fri, 2010-09-03 at 07:41 +0000, Andrea Gasparini wrote:
> > In my opinion this is something that shouldn't happen. If a user imports a SVG
> > file, probably (s)he'll want to keep the original image as is, without
> > overwriting it. The use case is: "As a user, I want to import a SVG file, so
> > that I can build my presentation on it", so it's not implicit that he wants to
> > trash his original SVG.
>
> Trash?! why trash? At the moment we don't make any modifications that
> stop users still use their SVG as an svg file. :)
> (that's not the case, but) even when we'll have a complete editor,
> it'll work as a normal SVG editor as well.
> So, users will still have their svg images, and their a4 presentation.

Modifying the original SVG image brings mainly three issues:

* If there's a bug in the code (and remember: we're not doing unit
  testing, so this is not a remote possibility), the resulting
  presentation may be damaged.

* If the image is modified, its hash will change too and won't match
  md5sums previously made by the user.

* If the image is on a CD or DVD, A4 will fail with an error.

"Import" is by definition a read-only operation. If you want to keep the
current behavior, please change the feature name to something like
"Upgrade image" or "Transform".

> > Ideally, we would use a different extension for presentations (e.g. .a4
> > instead of .svg) to avoid such confusion.
>
> Well, though the above sentences, I'm not completely opposed to have
> an our file type.
> Anyway, I see this as a different issue than the one addressed by the
> merge.
> Let's open a discussion on the ML, or on a bug report.

There's already a discussion on the mailing list.

> IMHO we have to decide just for the issue raise by Andrea G. (Save
> immediatly our metadata stub, or wait the user clicks on 'save'
> button?), and let the rest on other report and merge.

Yes, we can do that, but... don't we have already enough technical
debts?

> Modifying the original SVG image brings mainly three issues:

These three issues are present every time you save a file, whether you are importing it or just updating it. I don't see why the "importing" and the "updating" cases should be treated differently.

I think that (in both cases) the user should be given the opportunity to save to a new file or to overwrite the current one. I have filed bug #632748 about adding a "Save As" feature. Feel free to change its importance or milestone if you think it's a show-stopper.

> > IMHO we have to decide just for the issue raise by Andrea G. (Save
> > immediatly our metadata stub, or wait the user clicks on 'save'
> > button?), and let the rest on other report and merge.

I agree, let's try to find a consensus on this issue and merge this branch.

My opinion is that the file should be overwritten only when the user clicks "Save", for consistency with the behavior of "open" and "save" operations and to tackle said saving issues.

> Yes, we can do that, but... don't we have already enough technical
> debts?

I don't think that splitting an issue into smaller ones adds technical debt, as long as all sub-issues are scheduled for the same release.
Feel free to file and/or reschedule bugs if you think we are risking technical bankruptcy :-)

Ciao,

On Tue, 2010-09-07 at 22:51 +0000, Andrea Gualano wrote:
> > Modifying the original SVG image brings mainly three issues:
>
> These three issues are present every time you save a file, whether you
> are importing it or just updating it. I don't see why the "importing"
> and the "updating" cases should be treated differently.

For the first two cases (a bug in A4 and the hash), you are right: it's
the same by the point of view of the application. But if the user
explicitly click "Save", he's sure that the file will change. "Import"
leads people to think that nothing will change, because it is what
happens in every other program.

For the third case (read-only file or device) the thing is a bit more
complex:

* If Import requires write access to files, how can people import
  read-only files? They should copy them first to a writable location
  and/or set the W bit. This makes the assumption that the user has the
  time (and the knowledge) to do so and has enough file system
  permissions and space.

* Generally, when you open a read-only file with a GNOME application, it
  is opened in read-only mode: the Save button is disabled and
  "(read-only)" appears in the title bar. This means that the behavior
  of Save and Import can't be compared in this context. In fact Save is
  disabled because it makes no sense to use it, but Import is still
  useful.

> I think that (in both cases) the user should be given the opportunity
> to save to a new file or to overwrite the current one. I have filed
> bug #632748 about adding a "Save As" feature. Feel free to change its
> importance or milestone if you think it's a show-stopper.

I agree with you on this.

> > > IMHO we have to decide just for the issue raise by Andrea G. (Save
> > > immediatly our metadata stub, or wait the user clicks on 'save'
> > > button?), and let the rest on other report and merge.
>
> I agree, let's try to find a consensus on this issue and merge this
> branch.
>
> My opinion is that the file should be overwritten only when the user
> clicks "Save", for consistency with the behavior of "open" and "save"
> operations and to tackle said saving issues.

+1.

> > Yes, we can do that, but... don't we have already enough technical
> > debts?
>
> I don't think that splitting an issue into smaller ones adds technical
> debt, as long as all sub-issues are scheduled for the same release.
> Feel free to file and/or reschedule bugs if you think we are risking
> technical bankruptcy :-)

I said so because I'm a bit pessimist with community-driven projects.
The "as long as" is the part of the phrase that worries me. :-)

I see that sometimes (in big projects too) such inconsistencies and
misbehaviors are never fixed, although known, because considered less
"attractive" by developers or less important or too boring to fix. This
forces developers to refactor their code every so often and sometimes
leads to the creation of almost completely new projects with big
backward incompatibilities (a concrete example is Python 3k).

[ Hoping not to being too paranoiac ] Thanks,
Andrea

I should have adjusted some things:
* implemented 'save as' (merged warp10's branch).
* svg imported with a message, and not saved immediatly.
* if svg is readonly, save is disabled.
* used stock buttons for import message.
* dialog destroyed after the import.
* some text modified as Andrea Corb. suggested.