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.

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

Adds specific tools for selection tool #1620

Merged
merged 6 commits into from
Jul 10, 2017

Conversation

ketanhwr
Copy link
Contributor

Should the tool buttons be just fine? Because using along with modifiers feels a bit weird.

@bjorn
Copy link
Member

bjorn commented Jun 21, 2017

Should the tool buttons be just fine? Because using along with modifiers feels a bit weird.

The modifiers should definitely still work. What feels weird about it?

The only difference I noticed with GIMP is that GIMP will lock into a certain mode when a button is clicked, whereas when a modifier is held it will only temporarily switched to that mode. In contract, your patch currently always changes back to the "replace" mode when all modifiers are released. Maybe the locking would make it feel less weird?

@ketanhwr
Copy link
Contributor Author

Oh I previously misunderstood what you were trying to say. Then I tried out GIMP and got what you meant. The behaviour is similar now, have a look.

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks fine! I've added two inline comments, and don't forget to initialize mDefaultMode. :-)

mSelectionMode = Replace;
mReplace->setChecked(true);
mSelectionMode = mDefaultMode;
switch (mDefaultMode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make this switch based on mSelectionMode, you can move it out of the condition and remove all the setChecked(true) lines above. Then, all the bodies are just one line so you can even remove all the brackets. :-)

mActionGroup->addAction(mIntersect);

connect(mReplace, &QAction::triggered,
[this]() { mSelectionMode = mDefaultMode = Replace; });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some more indentation.

mAdd->setIcon(addIcon);
mAdd->setCheckable(true);
mAdd->setToolTip(tr("Add Selection"));
mReplace->setShortcut(QKeySequence(tr("Ctrl")));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, missed this earlier. That should be removed.

mReplace->setIcon(replaceIcon);
mReplace->setCheckable(true);
mReplace->setChecked(true);
mReplace->setToolTip(tr("Replace Selection"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please just call languageChanged at the end of the constructor, so you can remove all these lines here.

@bjorn
Copy link
Member

bjorn commented Jun 21, 2017

Found some nice alternative icons:

https://github.com/shimmerproject/elementary-xfce/blob/master/elementary-xfce/actions/22/selection-break.svg
https://github.com/shimmerproject/elementary-xfce/blob/master/elementary-xfce/actions/22/selection-union.svg
https://github.com/shimmerproject/elementary-xfce/blob/master/elementary-xfce/actions/22/selection-exclude.svg

Only problem is, there is no icon for "intersect", but maybe it won't be too difficult to derive it from these.

Also, as I said on IRC, these buttons should really also be added to the Magic Wand and the Select Same Tile tools. I think a similar approach as done for the Stamp Brush and Fill Tools would work, though actually I had plans to try writing an AbstractSelectionTool anyway...

@ketanhwr
Copy link
Contributor Author

ketanhwr commented Jun 21, 2017

Also, as I said on IRC, these buttons should really also be added to the Magic Wand and the Select Same Tile tools. I think a similar approach as done for the Stamp Brush and Fill Tools would work, though actually I had plans to try writing an AbstractSelectionTool anyway...

So what should I do as of now then?

@bjorn
Copy link
Member

bjorn commented Jun 21, 2017

So what should I do as of now then?

Well, you could introduce the AbstractSelectionTool now, just as way to share the implementation of these tool bar actions. I would later try to merge the move selection tool into it.

@bjorn bjorn merged commit 95d998c into mapeditor:master Jul 10, 2017
@bjorn
Copy link
Member

bjorn commented Jul 10, 2017

Seems like a really nice cleanup of duplicate functionality. Even better now that all selection tools have their selection modes visible!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants