Bug 5/30 Save Cancel Resets Modified Flag

Posted on Sep 29, 2020

Bug 5 is PixiEditor/PixiEditor/Clicking “Save” or “Save As…” and canceling, results in false “saved” state.

PixiEditor is a fun little WPF application that allows people to do things like pixel art.

This bug concerns the behaviour of the dirty flag after clicking Save or Save As: irrespective of whether the save action is cancelled, the dirty flag was always being set to false (i.e. not dirty and so no save necessary). This meant that the user was not getting prompted to save on exit if they had cancelled a previous save.

Having spent 4 years as the lead developer on a large WPF based application (I wrote the first lines of code, as well as many more) I know how a WPF application should look and how it should be structured. An important aspect of that is designing it so that it can be tested. WPF’s binding mechanism is designed to make implementing the MVVM paradigm easy. This separation, with a little care, means that the View Model layer can know nothing about the View layer. That’s not to say that they are decoupled. The View will always be tightly coupled to the View Model, but the View Model never needs to have any UI dependencies and so can be properly unit tested. The way to enforce this is to have separate View Model and View assemblies. The View Model assembly has no references to UI components and does not reference the View; the View however references the View Model. Likewise the Model has it’s own assembly and this does not reference View Model. In this way the Model assembly forms a natural API. It’s more complicated than this in practice of course, but that’s the theory.

On the application I worked on we wrote our own MVVM library. In my opinion it was far superior to any of the open source libraries available. It took a lot of inspiration from Caliburn Micro, but was much faster and didn’t implement any of Caliburn’s automagic stuff.1 There were also a number of extra features that we implemented based on the best parts of frameworks other members of the team had seen on other projects. The command routing was particularly good.

This project is what I would call a more traditional WPF application as it’s not designed to be tested. The bit of code I needed to modify was in a static class that was contained in a private method. In the static class there was a direct reference to the Windows SaveFileDialog API, which displays a user interface. The standard thing to do is to create an interface representing the dialog in the View Model layer and then to implement a facade around the SaveFileDialog in the View layer that gets injected in the bootstrapper. Nothing like this was going on, and trying to make this bit of code testable was both way beyond the scope of the bug, and would have been incongruent to the application design. So I decided simply to make the fix, do some manual due diligence on it and check in untested. Most of the application is untested, so there wasn’t much harm in this.

The PR can be found here.

Update

Boom! The PR got accepted first time.

Four down, 26 to go 😄


  1. The automagic stuff looks pretty cool when you first see it, and perhaps it makes sense for a very small project. However, when something fails to bind, or fails to work as expected, it can be a genuine challenge to work out how to resolve the issue. It also makes it much harder for other developers to work out what is going on because its automagic. Setting up bindings explicitly might be slightly more verbose, but in this case the verbosity pays dividends over the long term in terms of maintainability. ↩︎