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
Merged

Conversation

@ketanhwr
Copy link
Contributor

@ketanhwr ketanhwr commented Jun 21, 2017

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

ketanhwr added 2 commits Jun 21, 2017
@bjorn
Copy link
Member

@bjorn 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

@ketanhwr ketanhwr commented Jun 21, 2017

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

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) {

This comment has been minimized.

@bjorn

bjorn Jun 21, 2017
Member

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; });

This comment has been minimized.

@bjorn

bjorn Jun 21, 2017
Member

Needs some more indentation.

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

This comment has been minimized.

@bjorn

bjorn Jun 21, 2017
Member

Ah, missed this earlier. That should be removed.

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

This comment has been minimized.

@bjorn

bjorn Jun 21, 2017
Member

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

@bjorn
Copy link
Member

@bjorn 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 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 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.

ketanhwr added 3 commits Jul 7, 2017
@bjorn bjorn merged commit 95d998c into mapeditor:master Jul 10, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjorn
Copy link
Member

@bjorn 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.