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

Already on GitHub? Sign in to your account

Mouse Cursors and JPG Export Compression Quality Dialog: 06-28-12 #32

Merged
merged 11 commits into from Sep 29, 2012

Conversation

Projects
None yet
2 participants
Contributor

AndrewDavis commented Jun 28, 2012

Fully implemented Mouse Cursors and JPG Compression Export Quality Dialog (to the best of my knowledge).

Mouse Cursors: I've added (the code for) mouse cursors on the rest of the tools as well as a crosshair, circle, and cursor combination for the Paintbrush, Eraser, ColorPicker, and CloneStamp tools.

JPG Export Compression Quality Dialog: I've improved it so that the dialog remembers the previous JPG compression quality setting used, even after Pinta is restarted, the dialog is only displayed when Save As is clicked (or a Document is saved for the first time), the OK button is the default control on the dialog, and the dialog no longer disappears (on some OS's, like Windows XP) when Pinta is minimized/unfocused and then restored.

  • Andrew Davis, GSoC 2012

AndrewDavis added some commits Jun 16, 2012

Undid some code that doesn't work in Windows.
It only works in Linux, and I'm using Windows, so with Cameron's
approval I'm teporarily undoing this code until the problem is fixed (so
that I can continue working).
Added cursors and paintbrush thickness cursor.
Also fixed a null error that occurs in BaseBrushTool when the value of
BrushWidth is retrieved and the ComboBox hasn't been set up yet.
Implemented thickness icons on 4 tools.
Also added 2 methods to the BaseTool class for creating the icons used
for these 4 tools.
Improved JPG export compression quality dialog.
Now, the JPG export compression quality dialog:
- only displays when the user clicks Save As, not Save.
- remembers the previously exported JPG compression quality, even when
Pinta restarts.
- has the default control being the OK button.
- shouldn't disappear anymore (on some OS's, like Windows XP) when Pinta
is minimized/unfocused and then restored.

I'd rather just call GetSetting in the constructor of JpegFormat, and then we can avoid having hardcoded values like 101

Owner

cameronwhite replied Jun 28, 2012

Also, maybe define a constant for "jpg-quality", since it's also used in the PutSetting call.

Well, it didn't work. I figured it had to do with the JpegFormat class loading earlier than the SettingsManager or something. That's why I made it the way I did.

Okay, I'll make a constant - I guess I'll make 2: the first for 101, and the second for 85.

Owner

cameronwhite replied Jun 28, 2012

Ah, that's true, I didn't notice that.
You could just call GetSetting inside DoSave instead of having a jpgCompression member variable.

Also, I was meaning to have a constant for the "jpg-quality" string, not the value of 85.

Oh. I don't fully understand - is this for translation purposes, or do you just want it to be at the top of the class so that it can be easily seen and modified later if desired (or do you not even want it in the same class)?

Owner

cameronwhite replied Jun 28, 2012

Yeah, I just wanted it defined as a constant in this class so that it could be modified easily later if necessary.

It would be simpler to do this whenever a file is saved, instead of on exit. In particular, this approach is buggy if the user opens Pinta and closes it without saving a jpeg - they get 101 saved as their preferred jpg quality.

Why don't I just check to see if it's equal to 101, and if it is then I'll save 85 instead?

Owner

cameronwhite replied Jun 28, 2012

Well, I'd rather not have arbitrary constants like 101 floating around, and calls to PutSetting have essentially no performance impact (nothing is written to a file until SaveSettings is called).

Okay, will do.

I don't think this needs to be static - see below for my note about GetSetting and the 101 value.

You're right: that's a mistake.

This has a bug if the user saves a file, then does "Save As" and cancels - they get prompted for the jpeg quality the next time they select "Save"

Good call, I didn't see that possibility. I guess I'll just have to place it lower in the code when Pinta is for sure going to save it (I'll place it right before the code that leads to the showing of the dialog).

This is kinda confusing - Document.HasFile actually stores whether the document has been saved before (i.e. whether it has an associated file). This property is intended to store whether the file has been saved in the current session, right?

Perhaps I should explain it more in the comments?

It (referring to the choice of "HasBeenSaved") makes sense to me, because I've been thinking in terms of the file associated with the Document, not the Document itself. This tells whether or not the Document has already been saved to the particular file that it's associated with, which is not true in 2 cases:

  1. The Document has never been saved at all (it started out as a new Document and the user modified it).
  2. The user specifically clicked "Save As" (as opposed to Save) and chose a new file name, in which case the Document has technically never been saved (to the file associated with it).

Maybe I could rename (using the refactor tool) the variable to something like IsAssociatedWithSavedFile, although I think that's a little too long. :)

Owner

cameronwhite replied Jun 28, 2012

There's also other cases, such as opening an existing file, where HasFile is true but HasBeenSaved is false.

My main problem is that it's hard to figure out its meaning from looking at the name. What about HasBeenSavedInSession, or something like that? Either way, I think it should be explained more clearly in the comments.

Ehh, that can fit into #1 depending on how you look at it. :)

HasBeenSavedInSession works - will do.

Modified JPG compression quality setting usage.
Also improved comments and variables.

@cameronwhite cameronwhite commented on the diff Jun 29, 2012

Pinta.Core/Classes/Document.cs
@@ -99,6 +100,13 @@ public Document (Gdk.Size size)
public Guid Guid { get; private set; }
public bool HasFile { get; set; }
+
+ //Determines whether or not the Document has been saved to the file that it is currently associated with in the
@cameronwhite

cameronwhite Jun 29, 2012

Owner

This is a bit too verbose :) I think only the first two sentences are really necessary.

@AndrewDavis

AndrewDavis Jun 29, 2012

Contributor

The only reason I'll agree with you is because I had a chance to explain this in the SaveDocumentImplementationAction class. :) I'll reduce it to the first 2 sentences.

@cameronwhite cameronwhite commented on the diff Jun 29, 2012

Pinta.Core/Classes/DocumentWorkspace.cs
@@ -101,6 +101,12 @@ internal DocumentWorkspace (Document document)
CanvasSize = new Gdk.Size (new_x, new_y);
Invalidate ();
+
+ if (PintaCore.Tools.CurrentTool.CursorChangesOnZoom)
@cameronwhite

cameronwhite Jun 29, 2012

Owner

If you have two documents open, each with a different zoom setting, the cursor doesn't get updated if you switch to another document. You probably need an event listener somewhere for PintaCore.Workspace.ActiveDocumentChanged

@AndrewDavis

AndrewDavis Jun 29, 2012

Contributor

Ooh, I didn't know to test for that. I figured that in any other case, it would be redundant in setting the DefaultCursor or something (which would trigger the cursor change).

If the ActiveDocumentChanged event does not solve this problem, I think there's also the possibility of maybe the zoom not being updated in time or something. This seems possible because there are 2 differences: Document change and zoom change. It could potentially be caused by both in combination too. I'm only saying this in case the event doesn't work and I forget about this thought.

@cameronwhite

cameronwhite Jun 29, 2012

Owner

After looking in more detail, I think listening to the CanvasSizeChanged event would cover both the normal zoom case and the document switching case.

That event is triggered in MainWindow.ActiveDocumentChanged and from changing the CanvasSize on line 102 of DocumentWorkspace.

@AndrewDavis

AndrewDavis Jun 30, 2012

Contributor

Ok thanks!

@cameronwhite cameronwhite and 1 other commented on an outdated diff Jun 29, 2012

Pinta.Tools/Tools/CloneStampTool.cs
@@ -45,11 +45,34 @@ public class CloneStampTool : BaseBrushTool
get { return "Tools.CloneStamp.png"; }
}
public override string StatusBarText { get { return Catalog.GetString ("Ctrl-left click to set origin, left click to paint."); } }
- public override Gdk.Cursor DefaultCursor { get { return new Gdk.Cursor (PintaCore.Chrome.Canvas.Display, PintaCore.Resources.GetIcon ("Cursor.CloneStamp.png"), 6, 11); } }
+ private int iconOffsetX, iconOffsetY;
+ private int cursorOffsetX = 6;
@cameronwhite

cameronwhite Jun 29, 2012

Owner

Do some of these need to be private member variables? They're only used in one place.

@AndrewDavis

AndrewDavis Jun 29, 2012

Contributor

I presume you're referring to my variables - I'll look into it.

@AndrewDavis

AndrewDavis Jul 2, 2012

Contributor

Look at lines 105 and 108 of the DocumentWorkspace class. They're used outside of the BaseTool class, so yes, they need to be public. Also, if I change them to private/protected in the child classes, C# gives me an error because [A.] They don't match up with the public declaration and/or [B.] Virtual/Abstract variables can't be private.

@cameronwhite

cameronwhite Jul 2, 2012

Owner

Sorry, I wasn't very clear - I was asking if the new private member variables that you added were really necessary.

@AndrewDavis

AndrewDavis Jul 2, 2012

Contributor

Gotcha. No, I guess they weren't. I'll minimize the unnecessary code.

@cameronwhite cameronwhite and 1 other commented on an outdated diff Jun 29, 2012

Pinta.Tools/Tools/EllipseSelectTool.cs
@@ -42,6 +42,9 @@ public class EllipseSelectTool : SelectTool
public override string StatusBarText {
get { return Catalog.GetString ("Click and drag to draw an elliptical selection. Hold shift to constrain to a circle."); }
}
+
+ public override Gdk.Cursor DefaultCursor { get { return new Gdk.Cursor(PintaCore.Chrome.Canvas.Display, PintaCore.Resources.GetIcon("Menu.Edit.EraseSelection.png"), 8, 6); } }
@cameronwhite

cameronwhite Jun 29, 2012

Owner

I guess we'll need to find appropriate cursors for this tool and a few others?

@AndrewDavis

AndrewDavis Jun 29, 2012

Contributor

Yeah, for then I figured I would just add the code for a cursor and then make it an easy switch of a string and the location of the tip of the cursor... unless you want an X cursor! :)

@cameronwhite

cameronwhite Jun 29, 2012

Owner

That's fine. I just wanted to mention it in case you had forgotten :)

@cameronwhite cameronwhite commented on the diff Jun 29, 2012

Pinta.Core/Classes/BaseTool.cs
+ /// <summary>
+ /// Create a cursor icon with an ellipse that visually represents the tool's thickness.
+ /// </summary>
+ /// <param name="name">A string containing the name of the tool's icon to use.</param>
+ /// <param name="brushWidth">The current thickness of the tool.</param>
+ /// <param name="cursorWidth">The width of the tool's icon.</param>
+ /// <param name="cursorHeight">The height of the tool's icon.</param>
+ /// <param name="cursorOffsetX">The X position in the tool's icon where the center of the affected area will be.</param>
+ /// <param name="cursorOffsetY">The Y position in the tool's icon where the center of the affected area will be.</param>
+ /// <param name="ellipseColor1">The inner color of the ellipse.</param>
+ /// <param name="ellipseColor2">The outer color of the ellipse.</param>
+ /// <param name="ellipseThickness">The thickness of the ellipse that will be drawn.</param>
+ /// <param name="iconOffsetX">The X position in the returned Pixbuf that will be the center of the new cursor icon.</param>
+ /// <param name="iconOffsetY">The Y position in the returned Pixbuf that will be the center of the new cursor icon.</param>
+ /// <returns>The new cursor icon with an ellipse that represents the tool's thickness.</returns>
+ protected Gdk.Pixbuf CreateEllipticalThicknessIcon(string name, int brushWidth, int cursorWidth, int cursorHeight, int cursorOffsetX, int cursorOffsetY, Color ellipseColor1, Color ellipseColor2, int ellipseThickness, ref int iconOffsetX, ref int iconOffsetY)
@cameronwhite

cameronwhite Jun 29, 2012

Owner

Do we really need the ellipseColor1 and Color2 parameters? It seems like everyone is passing in the same colors.

@AndrewDavis

AndrewDavis Jun 29, 2012

Contributor

I don't know, I guess not. I think it will depend on whether or not every cursor uses the same color set. If you want, I can make these static values in the BaseTool class, perhaps.

@cameronwhite

cameronwhite Jun 29, 2012

Owner

That's fine, I think it's okay to leave it the way it is.

Owner

cameronwhite commented Jun 29, 2012

Also, a general issue I noticed: if I press Alt, the cursor seems to flicker a bit.

AndrewDavis added some commits Jul 2, 2012

Fixed Document switching cursor changing problem.
Also removed 2 sentences of comments that were unnecessary.
Removed custom mouse cursors from 3 tools.
The custom mouse cursors for the Gradient, Ellipse Select, and Rectangle
Select tools were removed. I had added some custom mouse cursor code for
5 tools earlier (including these 3), but it was decided that it would be
best to leave them with the standard mouse cursor for now.

cameronwhite added a commit that referenced this pull request Sep 29, 2012

Merge pull request #32 from mono-soc-2012/MouseCursorsJPGQuality
Mouse Cursors and JPG Export Compression Quality Dialog.

Conflicts:
	Pinta.Core/Extensions/CairoExtensions.cs

@cameronwhite cameronwhite merged commit d87315e into PintaProject:master Sep 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment