-
Notifications
You must be signed in to change notification settings - Fork 277
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
Inconsistent hotkey behaviour for select tools #144
Inconsistent hotkey behaviour for select tools #144
Conversation
…ore predictable. Added LassoSelectTool "S" shortcut.
Updating fork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this! I left a few notes in the comments about the code structure, but I also have a question about the behavior:
Should the index reset after changing to another tool group? For example, should pressing S, S, M, and then S again bring you to Rectangle Select rather than Ellipse Select?
Pinta.Core/Managers/ToolManager.cs
Outdated
if (group.Key.EqualsTo(shortcut)) | ||
{ | ||
//begin looking for the tool from the start | ||
if (group.Value.Item2 >= group.Value.Item1.Count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this extra section be avoided if the code below handled wrapping the index? (i.e. updating to (i + 1) % count
instead of just ++i
). I think the loop could also be avoided if the index is always valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. Made it as you say.
Pinta.Core/Managers/ToolManager.cs
Outdated
for (int i = 0; i < index; i++) { | ||
if (Tools[i].ShortcutKey.ToString ().ToUpperInvariant () == key) | ||
return Tools[i]; | ||
foreach (var group in groupedTools) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this loop be replaced with a direct lookup in the dictionary? (by making the dictionary always store the upper-case version of the key)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been thinking about it. I put some changes in order to simplify this, but it is not ready yet. I misunderstood the task. And now I can say that tuple is unnecessary, so more refactoring needs to be done. It will take a day more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem! The changes so far look good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a few bugs and fixed it. I hope now it works fine.
Pinta.Tools/CoreToolsExtension.cs
Outdated
@@ -66,6 +66,8 @@ public void Initialize () | |||
PintaCore.Tools.AddTool (new RoundedRectangleTool ()); | |||
PintaCore.Tools.AddTool (new EllipseTool ()); | |||
PintaCore.Tools.AddTool (new FreeformShapeTool ()); | |||
|
|||
PintaCore.Tools.CreateGoupedTools (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in the function name (missing r
for Gouped
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not relevant any more due to code changes.
Pinta.Core/Managers/ToolManager.cs
Outdated
@@ -77,7 +79,18 @@ public void RemoveInstanceOfTool (System.Type tool_type) | |||
} | |||
} | |||
} | |||
|
|||
|
|||
public void CreateGoupedTools() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be merged into the AddTool() and RemoveTool() methods - it's possible for add-ins to register additional tools, so the groupedTools
dictionary should be kept up to date in those methods instead of having a separate method to call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great idea! Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think the behavior is working well now!
Just had one comment for simplifying FindNextTool()
, and then I think this will be ready to merge!
Pinta.Core/Managers/ToolManager.cs
Outdated
PressedShortcutCounter = (i + 1) % groupedTools[shortcut].Count; | ||
return groupedTools[shortcut][i]; | ||
} | ||
else if (i == (PressedShortcutCounter % groupedTools[shortcut].Count)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here is a bit confusing to read - it might be cleaner to just deal with the cases separately
e.g.
if (LastUsedKey != shortcut)
{
// Return first tool in group.
PressedShortcutCounter = (1 % groupedTools[shortcut].Count);
LastUsedKey = shortcut;
return groupedTools[shortcut][0];
}
else
{
var tool = groupedTools[shortcut][PressedShortcutCounter];
PressedShortcutCounter = (i + 1) % groupedTools[shortcut].Count;
return tool;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks prettier! I had to add else if(...), because it didn't work otherwise, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Changed the way of choosing tools to make behavior of the shortcuts more predictable. Added for LassoSelectTool "S" shortcut.
Unfortunately, I didn't find out how to make Cyrillic letters work properly without dirty code, so I left that part as-is.