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
Merged

Selection of underlying objects #1494

merged 17 commits into from Mar 27, 2017

Conversation

Acuion
Copy link
Contributor

@Acuion Acuion 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 master branch 2 times, most recently from b7343a1 to c7c7863 Compare March 16, 2017 13:22
@bjorn
Copy link
Member

bjorn 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
Copy link
Contributor Author

Acuion 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.

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.

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;
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

Can just be called objectItemsAt.

if (levelNum == 0)
action->setEnabled(false);//just to set a starting point
else
action->setData(QVariant::fromValue(levelNum));
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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay, that's what I missed

if (!underlyingObjects.size())
break;
if (mSelectedRotationIndex >= underlyingObjects.size())
mSelectedRotationIndex = 0;//definitely another stack
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 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));
Copy link
Member

Choose a reason for hiding this comment

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

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"));
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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 meant reverting this signature change, please do because it doesn't matter in the end so keeping the patch smaller is better.

@Acuion
Copy link
Contributor Author

Acuion 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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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


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

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

Choose a reason for hiding this comment

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

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);
Copy link
Member

@bjorn bjorn Mar 20, 2017

Choose a reason for hiding this comment

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

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

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 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
Copy link
Member

Choose a reason for hiding this comment

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

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>
Copy link
Member

Choose a reason for hiding this comment

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

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());
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 meant reverting this signature change, please do because it doesn't matter in the end so keeping the patch smaller is better.

@Acuion
Copy link
Contributor Author

Acuion commented Mar 20, 2017

Fixed, but mSelectedInUnderlyingRotation is still here

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.

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);
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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)");
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Acuion Acuion Mar 20, 2017

Choose a reason for hiding this comment

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

Maybe add a numeration instead?
1) Name

Copy link
Member

Choose a reason for hiding this comment

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

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();
Copy link
Member

Choose a reason for hiding this comment

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

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

@Acuion
Copy link
Contributor Author

Acuion 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
Copy link
Member

bjorn 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
Copy link
Contributor Author

Acuion 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)
Copy link
Contributor Author

@Acuion Acuion Mar 21, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

@bjorn bjorn Mar 21, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Acuion Acuion Mar 22, 2017

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Acuion Acuion Mar 22, 2017

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Acuion commented Mar 24, 2017

@bjorn any new comments?

@bjorn
Copy link
Member

bjorn 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
Copy link
Contributor Author

Acuion 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 mapeditor:master Mar 27, 2017
@bjorn
Copy link
Member

bjorn 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
Copy link
Contributor Author

Acuion 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
Copy link
Contributor Author

Acuion 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
Copy link
Member

bjorn 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants