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

Selection of underlying objects #1494

Merged
merged 17 commits into from Mar 27, 2017

Conversation

@Acuion
Copy link
Contributor

commented Mar 16, 2017

My approach on #1491
User can iterate through objects by pressing Alt while hovering over them with cursor, also there is a new submenu in the context menu, which allows to select certain object in the stack.
I'm not sure about using new field 'mCurrMouseScenePosition' to get in-scene mouse coordinates in keyPressed method, is there any better way to do it?
And due to reselection of the topmost object while dragging, an underlying object can't be moved this way, only by arrows. Does it have to be fixed? If so, I need a clue how to do it right.

@Acuion Acuion force-pushed the Acuion:master branch 2 times, most recently from b7343a1 to c7c7863 Mar 16, 2017

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 17, 2017

Hey @Acuion, thanks for taking on this issue!

I'm not sure about using new field 'mCurrMouseScenePosition' to get in-scene mouse coordinates in keyPressed method, is there any better way to do it?

Why did you decide to cycle on Alt presses, rather than cycling on mouse clicks while Alt is held? That's how Inkscape does it and it would avoid this issue.

And due to reselection of the topmost object while dragging, an underlying object can't be moved this way, only by arrows. Does it have to be fixed? If so, I need a clue how to do it right.

You can drag an underlying selected objects by holding Alt, which will force the drag even if not hitting any object and prevents any changes to the selection. So no change should be needed there, apart from keeping that behavior working.

Acuion added 3 commits Mar 17, 2017
@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2017

Oh, yes, it was a quite odd decision, now should work as expected

if (modifiers & Qt::AltModifier) {...} is located outside the if (mClickedObjectItem) {...} block because when I click fast enough, something happens and mClickedObjectItem becomes nullptr.

@bjorn
Copy link
Owner

left a comment

if (modifiers & Qt::AltModifier) {...} is located outside the if (mClickedObjectItem) {...} block because when I click fast enough, something happens and mClickedObjectItem becomes nullptr.

Yeah, this double-clicking issue is something I've noticed as well. I would say don't try to avoid that problem in your code for now, because it should be fixed somewhere else.

See inline comments for the rest of my feedback.

duplicateObjects();
return;
}
return;

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 17, 2017

Owner

Please move the return back under the condition. The idea is to accept the event when a shortcut has been handled, which is not the case if just D is pressed, only on Ctrl+D.

For completeness sake and to avoid an easy mistake later on, feel free to add a break; instead.

@@ -133,9 +131,22 @@ ObjectGroup *AbstractObjectTool::currentObjectGroup() const
return dynamic_cast<ObjectGroup*>(mapDocument()->currentLayer());
}

QList<MapObjectItem*> AbstractObjectTool::listOfObjectItemsAt(QPointF pos) const

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 17, 2017

Owner

Can just be called objectItemsAt.

if (levelNum == 0)
action->setEnabled(false);//just to set a starting point
else
action->setData(QVariant::fromValue(levelNum));

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 17, 2017

Owner

If you put Q_DECLARE_METATYPE(Tiled::Internal::MapObjectItem*) at the bottom of mapobjectitem.h, then you can do action->setData(QVariant::fromValue(underlyingObjects[levelNum]) here and action->data().value<MapObjectItem*>() below.

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 18, 2017

Author Contributor

Yay, that's what I missed

if (!underlyingObjects.size())
break;
if (mSelectedRotationIndex >= underlyingObjects.size())
mSelectedRotationIndex = 0;//definitely another stack

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 17, 2017

Owner

I think instead of keeping track of the current "selection rotation index", the next selection should simply be based on the current selection. So basically, find the last selected object among the objects at the mouse position, and then take the next one (or cycle back to the first one if there is no next one).

It will be a bit more processing but I think it will make the system more predictable. At the moment there are some problems with the click sometimes not responding the first time you try it, or a deeper object getting selected than you might expect after having already used the feature somewhere else.

if (mSelectedRotationIndex >= underlyingObjects.size())
mSelectedRotationIndex = 0;//definitely another stack
selection.clear();
selection.insert(underlyingObjects.at(mSelectedRotationIndex));

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 17, 2017

Owner

Note that Tiled adds to the selection when Shift or Control modifiers are held. The same should apply here, so you can use Shift or Control to add below objects to the selection.

I have a hunch, btw, that you can re-use the code below by just changing the mClickedObjectItem to the next underlying object.

if (underlyingObjects.size() > 1) {
menu.addSeparator();

QMenu *selectUnderlyingMenu = menu.addMenu(tr("Select an underlying object"));

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 17, 2017

Owner

What do you think about rather than adding the items here, showing an entirely different menu with just the list of objects, when right-clicking while holding Alt?

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 18, 2017

Author Contributor

To separate object-related actions and a selection is a good idea. But where is it better to place it, in the abstract or in the selection tool? On the one hand, in the abstract it can be accessed from all tools, on the other, alt+click is working only with the selection one, so it will be more logical.

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

My first thought is that indeed it would be more logical to only supports this alternative menu in the ObjectSelectionTool.

@@ -115,8 +116,7 @@ void AbstractObjectTool::mouseMoved(const QPointF &pos,
void AbstractObjectTool::mousePressed(QGraphicsSceneMouseEvent *event)
{
if (event->button() == Qt::RightButton) {
showContextMenu(topMostObjectItemAt(event->scenePos()),
event->screenPos());
showContextMenu(event->scenePos(), event->screenPos());

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 18, 2017

Author Contributor

Should I return it back? Btw, I think it is more flexible

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Return what back? The menu? That's not possible because the menu is destroyed before the function returns. If we need it, we should have a createContextMenu, but I think we don't need it.

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 20, 2017

Author Contributor

The line with showContextMenu. Because the change from topMostObjectItemAt to event->scenePos() is no longer required.

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

If you meant reverting this signature change, please do because it doesn't matter in the end so keeping the patch smaller is better.

Acuion added 2 commits Mar 18, 2017
@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

@bjorn I don't know do I have to comment to indicate it, but a new version is ready

  • a new context menu in the selection tool
  • shift&ctrl support in both alt+click and the menu
case Qt::RightButton:
if (event->modifiers() & Qt::AltModifier) {
QList<MapObjectItem*> underlyingObjects = objectItemsAt(event->scenePos());
if (underlyingObjects.size() > 1) {

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 19, 2017

Author Contributor

Perhaps the menu should always be shown, if the first line is now active

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Yes, I think the check should be if (!underlyingObjects.isEmpty()) {.

case Qt::RightButton:
if (event->modifiers() & Qt::AltModifier) {
QList<MapObjectItem*> underlyingObjects = objectItemsAt(event->scenePos());
if (underlyingObjects.size() > 1) {

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Yes, I think the check should be if (!underlyingObjects.isEmpty()) {.


QAction *action = selectUnderlyingMenu.exec(event->screenPos());

if (MapObjectItem* objectToBeSelected = action->data().value<MapObjectItem*>()) {

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

This crashes if action is null (no action chosen / clicked outside of menu).

if (underlyingObjects.isEmpty())
break;
auto currRotation = std::find(underlyingObjects.begin(),
underlyingObjects.end(), mSelectedInUnderlyingRotation);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Please prefer to use Qt methods when available. This should just be underlyingObjects.indexOf(mSelectedInUnderlyingRotation).

However, I suggested to not use any other state than the current selection, to make this action completely predictable. There should not be a mSelectedInUnderlyingRotation, but rather you could search for the bottom-most selected object and then select the next one (cyling back to top-most object if it doesn't exist).

It seems reverse iterators have only been added to QList in Qt 5.6, and Tiled currently needs to compile with Qt 5.4, so I think there will be not such a short way of finding the bottom-most selected object. It would probably just be easiest to do something like:

MapObjectItem *nextItem = underlyingObjects.first();
for (int i = underlyingObjects.size() - 1; i >= 0; --i) {
    MapObjectItem *underlyingObject = underlyingObjects.at(i);
    if (selection.contains(underlyingObject))
        break;
    nextItem = underlyingObject;
}

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 20, 2017

Author Contributor

I tried this approach and realized that it is not compatible with a shift selection. If you start Shift+Alt+Click'ing in the middle of the stack, once bottom-most element is included to the selection, nextItem will always be the first element in the stack instead of continue iterating.
Also it is not compatible with a shift unselection - in this case we need to access the next selected object.

currRotation = underlyingObjects.begin();//new stack of objects
else {
if ((modifiers & (Qt::ShiftModifier | Qt::ControlModifier)) == 0)
selection.remove(mSelectedInUnderlyingRotation);//deselect previous

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Instead of doing this, please suppress the "Clicking one of the selected items changes the edit mode" below when Alt is held. Unless I'm missing something, that should do the same in less code.

@@ -25,6 +25,7 @@
#include <QList>
#include <QSet>
#include <QVector>
#include <QMenu>

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Please move that include to the objectselectiontool.cpp file.

@@ -115,8 +116,7 @@ void AbstractObjectTool::mouseMoved(const QPointF &pos,
void AbstractObjectTool::mousePressed(QGraphicsSceneMouseEvent *event)
{
if (event->button() == Qt::RightButton) {
showContextMenu(topMostObjectItemAt(event->scenePos()),
event->screenPos());
showContextMenu(event->scenePos(), event->screenPos());

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

If you meant reverting this signature change, please do because it doesn't matter in the end so keeping the patch smaller is better.

Acuion added 2 commits Mar 20, 2017
@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

Fixed, but mSelectedInUnderlyingRotation is still here

@bjorn
Copy link
Owner

left a comment

I tried this approach and realized that it is not compatible with a shift selection. If you start Shift+Alt+Click'ing in the middle of the stack, once bottom-most element is included to the selection, nextItem will always be the first element in the stack instead of continue iterating.
Also it is not compatible with a shift unselection - in this case we need to access the next selected object.

Ok, this is not the behavior I observe in Inkscape, but I can see it being interesting in some way, though I think the use-case would be very rare. And there are some problematic scenarios:

  1. With a stack of 2 items, use Alt to select an object below another object.
  2. Select the top-most object without holding Alt.
  3. Now try to select the object below while holding Alt, and notice how it will not change the selection at all on the first click.

The last thing happens because mSelectedInUnderlyingRotation is still set to the other item in the stack, and mClickedObjectItem gets set to the already selected item.

  1. Have a stack of 3 and select the second item in the stack by clicking an area not occupied by the topmost item.
  2. Try to select the item beneath using Alt clicking, but in an area also occupied by the topmost item. Notice how the topmost item gets selected, rather than the item below.

In general, the user will expect the next selection to be based on the current selection. I think this predictability is more important than the behavior you've implemented. Maybe you have a solution to these scenarios? I considered maybe always synchronizing mSelectedInUnderlyingRotation to mClickedObjectItem in mouseReleased, but that would still cause problems when Shift or Control was held to remove items from the selection. But unless there is a straight-forward solution, I would prefer to remove this special rotation logic.

// Clicking one of the selected items changes the edit mode
setMode((mMode == Resize) ? Rotate : Resize);
if (modifiers & Qt::AltModifier) {
selection.remove(mClickedObjectItem);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Actually I just realized this is not necessary at all. selection isn't used anymore after this, so please remove this check here again. And I think there is no need for suppressing the mode toggle when Alt is pressed with a single item below the mouse.

mapScene()->setSelectedObjectItems(selection);
}
}
else

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Coding style: since the connected if already has brackets, please add them here as well, and put them on the same line, like } else {.

auto selection = mapScene()->selectedObjectItems();
if (event->modifiers() & (Qt::ShiftModifier | Qt::ControlModifier)) {
if (selection.contains(objectToBeSelected))
selection.remove(objectToBeSelected);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Coding style: please fix the indentation here (only 4 spaces needed instead of 8).

for (int levelNum = 0; levelNum < underlyingObjects.size(); ++levelNum) {
const QString& objectName = underlyingObjects[levelNum]->mapObject()->name();
QString actionName = (objectName.isEmpty() ? tr("Object at level %n", "", levelNum + 1) : objectName)
+ tr(levelNum ? "" : " (topmost)");

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Localization: I think lupdate won't know what to do with a conditional being passed into tr. You should do levelNum ? QString() : tr(" (topmost)"), or wrap the appending with an if. Translation wise, there is of course the problem of some languages wanting this part on the other side. Maybe we can just leave it out.

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 20, 2017

Author Contributor

Maybe add a numeration instead?
1) Name

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

Sure, that would be fine. Then you can even do "&1) " for the first 9 entries to enable the number keys to be pressed for selecting one of the entries (& sets the mnemonic).

if (mClickedObjectItem) {
QSet<MapObjectItem*> selection = mapScene()->selectedObjectItems();

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

You don't use the selection anymore outside of this condition, so please move it back here.

@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

So, do you think that multiple in-stack selection can be removed from the alt+click logic (a user will still can append an item from the stack to an existing selection)? A new option "select all" in the context menu can be added instead. Other selection combination can be done with a shift+context menu selecting.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 21, 2017

So, do you think that multiple in-stack selection can be removed from the alt+click logic (a user will still can append an item from the stack to an existing selection)?

I'm not sure what you mean by this. I don't think I thought anything in addition to what I wrote.

A new option "select all" in the context menu can be added instead. Other selection combination can be done with a shift+context menu selecting.

While you could add a "Select All" option, I'm not sure if it will be very useful. If you want to select all the items, then you can already just drag a regular selection rectangle (possibly while holding Shift to force selection in case you need to start dragging on top of one of the items).

@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

  • A numeration in the context menu
  • The rotation is now based on a selection
    • Shift unselection is not supported in this mode

for (int levelNum = 0; levelNum < underlyingObjects.size(); ++levelNum) {
const QString& objectName = underlyingObjects[levelNum]->mapObject()->name();
QString actionName = QLatin1String(levelNum < 9 ? "&" : "") + tr("%n) ", "", levelNum + 1)

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 21, 2017

Author Contributor

Is there any better way to construct a "%d) " string in this case? I think it doesn't require any translation, so can be made without tr.
And how to properly update a .ts file? I can provide a russian translation for strings in my code (now it is only "Unnamed object")

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

I mostly just make it a translatable string anyway, because you never know (especially since some languages have different writing directions). But since this number does not relate to a plural form, you should not use the plural form override of tr.

I would probably just do:

if (objectName.isEmpty())
    objectName = tr("Unnamed object");

QString actionName;
if (levelNum < 9)
    actionName = tr("&%1) %2").arg(levelNum + 1).arg(objectName)
else
    actionName = tr("%1) %2").arg(levelNum + 1).arg(objectName)

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 22, 2017

Author Contributor

Ok, but what about a .ts file update? Can it be done manually or only via lupdate?

if (selection.contains(mClickedObjectItem))
selection.remove(mClickedObjectItem);
if (!(modifiers & Qt::AltModifier) && selection.contains(mClickedObjectItem))
selection.remove(mClickedObjectItem);// Removal is not supported in alt+click mode

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

I don't understand why you would block removal. I don't think there is a negative side-effect to it.

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 22, 2017

Author Contributor

Otherwise, if the whole stack is selected, it select and unselect the same object in a cycle. I think it looks buggy

do lastSelectedIndex = (lastSelectedIndex + 1) % underlyingObjects.size();
while (selection.contains(underlyingObjects.at(lastSelectedIndex))
&& lastSelectedIndex != underlyingObjects.size() - 1);
mClickedObjectItem = underlyingObjects.at(lastSelectedIndex);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

Was something wrong with the loop I wrote earlier? I think it was easier to understand and more efficient than this approach.

This comment has been minimized.

Copy link
@Acuion

Acuion Mar 22, 2017

Author Contributor

It doesn't support a last->first->second iteration. If the bottom part of the stack is Shift-selected, your code always point at the topmost element.

} else {
selection.clear();
selection.insert(objectToBeSelected);
}
mapScene()->setSelectedObjectItems(selection);
}
}
else
} else

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

Coding style: if some if/else chain has brackets somewhere, it should have them everywhere. Please add { here.


for (int levelNum = 0; levelNum < underlyingObjects.size(); ++levelNum) {
const QString& objectName = underlyingObjects[levelNum]->mapObject()->name();
QString actionName = QLatin1String(levelNum < 9 ? "&" : "") + tr("%n) ", "", levelNum + 1)

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

I mostly just make it a translatable string anyway, because you never know (especially since some languages have different writing directions). But since this number does not relate to a plural form, you should not use the plural form override of tr.

I would probably just do:

if (objectName.isEmpty())
    objectName = tr("Unnamed object");

QString actionName;
if (levelNum < 9)
    actionName = tr("&%1) %2").arg(levelNum + 1).arg(objectName)
else
    actionName = tr("%1) %2").arg(levelNum + 1).arg(objectName)
@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2017

@bjorn any new comments?

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 24, 2017

@Acuion I can only commit my time fully to Tiled two days per week (currently Monday and Tuesday). On other days I may have no time at all, so while I will try to respond earlier, it may take until Monday.

About the select and unselect, I think it's better than no response at all, but it's also because I've seen the same behavior in Inkscape.

If the bottom part of the stack is Shift-selected, your code always point at the topmost element.

Again, that was intentional and matches the behavior in Inkscape. I will need some time to try out your code again to see what really the difference is and what behavior I'd prefer.

@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2017

Ok, thank you :)
I just want the patch to be finished asap. Waiting for your next feedback.

@bjorn bjorn merged commit 71ce959 into bjorn:master Mar 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 27, 2017

@Acuion After looking at your code in detail, I liked the fact that it looks for the first non-selected object when no unselected object was available at the bottom. I did however rewrite that part a little, since I think it was rather hard to see what it was doing.

I did allow removal from the selection, so that the user doesn't have to be so conscious about releasing his Alt key when he wants to exclude some object. Indeed, it can only work for the first object and as long as there is no unselected object below, but I don't think it's a reason to block it entirely since I think this case isn't so rare.

Thanks a lot for your contribution!

@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2017

Thanks for your guidance! Yes, I think that unselection of the topmost element really has its application.

Btw, you didn't explained how to modify the translation. This patch is already meged, but it will be useful in the future, so I will be grateful for the explanation :).

PS: I've shared my draft via the GSoC system. Waiting for the feedback

@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2017

And seems there is a little bug in your code
for (int i = 1; i < underlyingObjects.size() - 2; ++i) {
probably should be
for (int i = 1; i < underlyingObjects.size() - 1; ++i) {
or <=

In stack of three select the last element, shift+alt+click twice and realize that the second one never get the selection.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 28, 2017

Btw, you didn't explained how to modify the translation. This patch is already meged, but it will be useful in the future, so I will be grateful for the explanation :).

That is because I don't update translation files until about 2 weeks before a new release. This 2 week period is known as a "string freeze", in which no changes to translated strings are allowed. This gives all translators a chance to update the translations (and you could help update yours).

And seems there is a little bug in your code

Thanks for noticing! I only meant to exclude the last element, but subtracted one too much. :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.