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.

Sign up for GitHub

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

Jump to bottom

Inconsistent hotkey behaviour for select tools #144

Merged
merged 7 commits into from
Sep 11, 2020
Merged

Inconsistent hotkey behaviour for select tools #144

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5cfa2c9
Changed the way of choosing tools to make behavior of the shortcuts m…
thekolian1996 Sep 7, 2020
39cc8f1
Merge pull request #2 from PintaProject/master
thekolian1996 Sep 7, 2020
1845530
Merge branch 'master' into feature/B-1430794_PropertyOnLayer
thekolian1996 Sep 7, 2020
a59523b
B-1558767 Refactor dictionary of groupedTools to keep it up-to-date.
thekolian1996 Sep 8, 2020
fa9d8a9
B-1558767 Refactored FindNextTool() method in order to simplify it.
thekolian1996 Sep 9, 2020
4317a01
B-1558767 Removed tuple because of its uselessness, created one unite…
thekolian1996 Sep 10, 2020
f99b009
B-1558767 Refactoring.
thekolian1996 Sep 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 51 additions & 15 deletions Pinta.Core/Managers/ToolManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,15 @@ public class ToolManager : IEnumerable<BaseTool>
int prev_index = -1;

private List<BaseTool> Tools;
private Dictionary<Gdk.Key, Tuple<List<BaseTool>, int>> groupedTools;

public event EventHandler<ToolEventArgs> ToolAdded;
public event EventHandler<ToolEventArgs> ToolRemoved;

public ToolManager ()
{
Tools = new List<BaseTool> ();
groupedTools = new Dictionary<Gdk.Key, Tuple<List<BaseTool>, int>>();
}

public void AddTool (BaseTool tool)
Expand Down Expand Up @@ -77,7 +79,18 @@ public void RemoveInstanceOfTool (System.Type tool_type)
}
}
}


public void CreateGoupedTools()
Copy link
Member

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.

Copy link
Contributor Author

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.

{
foreach (var tool in Tools)
{
if (!groupedTools.ContainsKey(tool.ShortcutKey))
groupedTools.Add(tool.ShortcutKey, new Tuple<List<BaseTool>, int>(new List<BaseTool>(), 0));

groupedTools[tool.ShortcutKey].Item1.Add(tool);
}
}

void HandlePbToolItemClicked (object sender, EventArgs e)
{
ToggleToolButton tb = (ToggleToolButton)sender;
Expand Down Expand Up @@ -172,23 +185,32 @@ public void SetCurrentTool (Gdk.Key shortcut)
if (tool != null)
SetCurrentTool(tool);
}

private BaseTool FindNextTool (Gdk.Key shortcut)
{
string key = shortcut.ToString ().ToUpperInvariant ();

// Begin looking at the tool after the current one
for (int i = index + 1; i < Tools.Count; i++) {
if (Tools[i].ShortcutKey.ToString ().ToUpperInvariant () == key)
return Tools[i];
}

// Begin at the beginning and look up to the current tool
for (int i = 0; i < index; i++) {
if (Tools[i].ShortcutKey.ToString ().ToUpperInvariant () == key)
return Tools[i];
foreach (var group in groupedTools)
Copy link
Member

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)

Copy link
Contributor Author

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.

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! The changes so far look good

Copy link
Contributor Author

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.

{
if (group.Key.EqualsTo(shortcut))
{
//begin looking for the tool from the start
if (group.Value.Item2 >= group.Value.Item1.Count)
Copy link
Member

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.

Copy link
Contributor Author

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.

{
groupedTools[shortcut.ToUpper()] = new Tuple<List<BaseTool>, int>(group.Value.Item1, 1);
return group.Value.Item1[0];
}
//iterating through the group of tools with the same shortcut
for (int i = 0; i < group.Value.Item1.Count; i++)
{
if (i == group.Value.Item2)
{
groupedTools[shortcut.ToUpper()] = new Tuple<List<BaseTool>, int>(group.Value.Item1, ++i);
return group.Value.Item1[group.Value.Item2];
}

}
}
}

return null;
}

Expand Down Expand Up @@ -242,4 +264,18 @@ protected override void OnBuildToolBar (Toolbar tb)
tool_sep = null;
}
}

//Key extensions for more convenient usage of Gdk key enumerator
public static class KeyExtensions
{
public static bool EqualsTo(this Gdk.Key k1, Gdk.Key k2)
{
return k1.ToString().ToUpperInvariant() == k2.ToString().ToUpperInvariant();
}

public static Gdk.Key ToUpper(this Gdk.Key k1)
{
return (Gdk.Key)Enum.Parse(typeof(Gdk.Key), k1.ToString().ToUpperInvariant());
}
}
}
2 changes: 2 additions & 0 deletions Pinta.Tools/CoreToolsExtension.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ();
Copy link
Member

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)

Copy link
Contributor Author

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.

}

public void Uninitialize ()
Expand Down
3 changes: 2 additions & 1 deletion Pinta.Tools/Tools/LassoSelectTool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ public LassoSelectTool ()
public override string Name { get { return Catalog.GetString ("Lasso Select"); } }
public override string Icon { get { return "Tools.LassoSelect.png"; } }
public override string StatusBarText { get { return Catalog.GetString ("Click and drag to draw the outline for a selection area."); } }
public override Gdk.Cursor DefaultCursor { get { return new Gdk.Cursor (Gdk.Display.Default, PintaCore.Resources.GetIcon ("Cursor.LassoSelect.png"), 9, 18); } }
public override Gdk.Key ShortcutKey { get { return Gdk.Key.S; } }
public override Gdk.Cursor DefaultCursor { get { return new Gdk.Cursor (Gdk.Display.Default, PintaCore.Resources.GetIcon ("Cursor.LassoSelect.png"), 9, 18); } }
public override int Priority { get { return 9; } }
#endregion

Expand Down