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

Feature #9094: Del-key performs 'delete selected features' #1010

Merged
merged 13 commits into from Dec 30, 2013

Conversation

Projects
None yet
6 participants
@ahuarte47
Copy link
Contributor

ahuarte47 commented Nov 25, 2013

No description provided.

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 2, 2013

I have tried to bring together all the comments and I have updated the pull request:

  • Selection tools (With point, rectangle, circle...) delete selected features with 'backspace' or 'delete' key without confirmation (It is an undoable action).
    Also, a new status message notify the number of features deleted.
  • DEL key in attribute table delete the seleted features similarly.
  • Node tool, or capture tool, remove a node with 'backspace' or 'delete' key.
  • TOC remove selected layer[s] with Ctrl+D with ask for confirmation (It is not an undoable action).
    Also, a new status message notify the number of layers removed.
@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Dec 2, 2013

@ahuarte47 I tested the PR here locally, seems to work fine!

two comments:

  1. If I create a NEW polygon, I can/could already remove the last added node by clicking 'backspace'. BUT delete is NOT working there. It would be great if there delete would also have the same function. So if possible can delete also remove last added node there?

  2. related to this: in the node-tool modus, after deleting a node with either delete or backspace, there is no 'current/selected node' anymore. So it is not possible to hit 3 times delete to delete the three adjacent nodes.

Does it sound logical to add these two points also?

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 2, 2013

  1. If I create a NEW polygon,....
    Oops, here when I am drawing a new polygon 'del' key remove last node as 'backspace' does.

  2. related to this: in the node-tool modus...
    Yes I understand it, you ask me that two adjacent nodes to be auto-selected, go to consecutively deleting nodes. But you think that's widely used?, I guess the user will want to delete the current before or after and not often the two. Although I can be wrong.

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Dec 2, 2013

@ahuarte47 you are right. 1) is working here too. Sorry for the noice

ad 2) No, what I mean is that it would work like when you create a new polygon. There if you create 5 nodes, you hit DEL the last one is deleted. When you then hit DEL again, the node before that one (current last one) is deleted etc. So hitting 5 times DEL removes the whole polygon.
When in the node-tool modus. If I select one node (it turns blue with me) and I hit DEL, then that node is deleted, but there is not another node deleted after that, so I cannot hit DEL again (I can but nothing happens).
So what I propose: is after removing the selected node, select one node next to it (not sure which one of the two neighours it should be then: the one before (looking at drawing order). But what to do then if it was the FIRST node....
Well, maybe it is just too complex. And we should start with this....

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 2, 2013

Don't worry Richard, you're bringing me great ideas.

About (2), I had already understood this, I can implement auto-select previous node, (When you rearch first, I can go to last node and so on). But the problem is what node select, previous or next.

I propose select previous, but if you agree, I can catch other keys, by example, '+' to select next node to the current node, or '-' to select previous node. Then, the user can easily select the desired node, ....

What you think ?
Best regards

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Dec 2, 2013

I would think no more keys. Just take the previous node.

More ideas :-) well:

  • if we now have clicking del/backpace removing of current selection. Do we really need the 'remove selection' button then, it is obvious to try del first I think? A word processor or inkscape does not have these anymore either?
  • if you are really into the edit tools now: if I select a feature, and then select 'Move feature(s)' button I can move the feature by just click/dragging wherever I want. While intuitively from other programs, I think the feature should only move when you start dragging INTO the feature (so really 'drag the feature'). And if I click somewhere else (OUT of the feature) the selection is removed.... Or when I click in another feature, the selection goes to that feature (or ctrl-clicking in a feature adds that feature to the selection).

(but maybe I should make this a new feature request :-) )

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 2, 2013

I am sorry, I think with '+' and '-' node selection can be very agile. These keys only would be active when the 'node tool' is active.

About 'remove selection' I can add a new button if you want.

About 'move features', I have tested it, (I am new in QGIS :-)) and I humbly think that it can be improved. In other app desktops, the user select the target features, then move they drawing a line (it indicates the displacement) and optionally using the "snap_to_vertex" configuration.

Best regards

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 2, 2013

Hi Richard, The user can deselect the current active node of a feature clicking in any map area where there is not other vertex. The 'remove selection' button may not be necessary.

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 3, 2013

Hi Richard, I have updated the PR with two last commits:

ahuarte47@1f1bbce
Automatically selects the adjacent vertex to last removed selected node.

ahuarte47@d548e1f
'+' and '-' select next and previous to current active node respectively. This commit is for demo proposes and shows my proposal for fast selection of nodes. If you think that it must not be pulled, I will undo it.

Regards

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Dec 3, 2013

@ahuarte47 The +/- behaviour sounds great to me - I'd love to see it included in this pull.

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Dec 3, 2013

tested, working and I think looking good :-)

two small things:

  • if I look into 'Settings/Configure shortcuts' at 'Delete selected' I do not see 'Delete' or 'Backspace'. Not sure if there is a connection between the list there and the code, or that this is a hand crafted list. But I think it should match these (now hardcoded?) shortcuts? Or is is just not possible to change these via the Configure shortcuts anymore?
  • we/I should write something about this in the docs before the pull I think (otherwise these are hidden features) Somewhere around http://docs.qgis.org/testing/en/docs/user_manual/working_with_vector/editing_geometry_attributes.html Though I'm not sure, if documentation should have ALL possible combinations of shortcuts or actions written out....

and I had a strange thing ones, when I selected several nodes with the node tool (using the ctrl button) and deleted those 3 nodes via the delete button. Then the middle one was not deleted and kept selected. But I can not reproduce this anymore...

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Dec 3, 2013

mmm, talking to some colleagues here.

for the plus and minus keys here on my keyboard I actually have to click the 'minus' and/or the 'shift-equalssign'. While for example in inkscape you use: tab and shitftab to move to to next and previous node.
Sounds more intuitive to me? Other opinions?

Another idea (again). IF I have a node selected (in Node tool) should it not be handy if I can move this one node with the arrow-keys (say: 2 pixels for only arrow, and 5 pixels for shift-arrow). Maybe only when you have 1 node, otherwise it becomes too complex I think.
Off course it would be nice also if I could move the feature with this. (though currently you pan when you use the arrow keys)

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 3, 2013

Hi Richard, about 'Settings/Configure shortcuts': I think that this dialog only shows shortcuts for generic actions or for start tools, some of them are configured by default in related *.ui files in QGIS source code. PR 1012 that you has merged recently is an example.

The new shorcuts of node tool, and other existing keys in several tools (QgsMapToolCapture, QgsGrassMapcalc, QgsMapToolNodeTool, QgsMapToolAnnotation...) are hardcoded as private shorcuts of each tool. I don't know if documented.

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 3, 2013

About plus and minus keys, on my keyboard in windows I only need click these single keys. But if you prefer other more coherent keys, (tab and shitftab), I agree with it. If other opinions are not contrary, I gladly change.

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 3, 2013

About move the node or feature with arrow keys: I can implements this, but I don't know if it is very useful move a geometry in pixel coordinates (I say this with all due respect :-)). In my experience, the user needs to move a geometry in map coordinates using a precise offset, from keyboard or a drawing a line-displacement using optionally existing nodes as base.

If you want, we can create a new feature request for modify the current "move tool" behavior:

An initial proposal: 'move tool' works with the current selected features collection, the user move these features, not those clicked in first mousedown. It would allow that the user can draw a precise offset using existing nodes as base (snapping), and/or also create a new dialog where the user can input the specific offset.

But first I must study if other plugins contain this "move tool" (http://plugins.qgis.org/plugins/DigitizingTools, http://plugins.qgis.org/plugins/cadtools, http://plugins.qgis.org/plugins/numericalVertexEdit...) and keep everything as is.

Best regards

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Dec 3, 2013

let's not touch the move tool now... otherwise this PR will never be pulled....

one 'strange' behaviour I notice: if I select features via the 'Select features using an expression', then they 'look selected' == yellow, but I cannot delete them with delete/backspace.

is it possible to make the delete/backspace button also work on features selected via an expression?

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Dec 3, 2013

I personally don't think tab/shift tab are a good choice, since they have very specific use in UIs (navigating through GUI controls). How about something like [ and ]? They require no key combination, won't clash with anything else, and it's nice and clear which one is forward and which is back...

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 3, 2013

Hi Nyall, thank for your advice.
I do not want, and I can not, to impose on the decision, also I'm very influenced by windows applications :-)

Summarizing, we can use:
'+' '-'
'tab' 'shift-tab'
'[' ']'
'<' '>' ????
'Re Pag' 'Av Pag', already used in mapcanvas
'Left arrow' 'Right arrow', already used in mapcanvas
'Up arrow' 'Down arrow', already used in mapcanvas

Waiting opinions of experts in usability ;-)

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 3, 2013

Hi Richard, about:

"one 'strange' behaviour I notice: if I select features via the 'Select features using an expression', then they 'look selected' == yellow, but I cannot delete them with delete/backspace"...

I see the problem, the features are selected, but the 'delete-backspace' keys only are executed when a selection tool is active (By point, rectangle, ....). If you has other active tool (pan, zoom, node tool...) then these keys are executed in the context of this tool and can be executed other code. For example, when 'node tool' is active 'del' key try delete selected nodes and not selected features. I managed to explain?, My English is bad :-(

You are right, the user fails to understand why they are not deleted. We can provide one solution:

Activate a selection tool after the "selection by expression" dialog is closed (It is easy in QgsExpressionSelectionDialog.cpp). But it can be not acepted by QGIS community.

Regards

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Dec 4, 2013

@nyalldawson though I agree that tab is used for navigating through gui/dialogs, it is in most software also used to walk through elements. Inkscape (I think that is an example of mature editing software) also uses tab and shift tab to go through the nodes of given element.
But if I'm the only one: democracy helps :-)

@ahuarte47 About the selection by expression problem. I think we should not selection silently active.
Had an IRC chat with @matthias-kuhn about this. And our conclusion was that the optimal solution would be that the delete/backspace key is only active when there is at least one selected feature in current layer.
Actually in the same loop as which generates the message 'current layer is not active'...
Not sure if that is doable though...

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 4, 2013

Ok, Richard, I can do your advice about 'selection by expression' problem. But I think there is a problem, if we use the 'delete' key always to delete selected features, may be that there is other active tool catching 'delete' key for other purposes ('node' tool for example) and conflict occurs.

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Dec 4, 2013

No strong opinion from my side, only proposals to heavy-users of digitizing

There are different concepts: Mapping the del-Key to a MapTool or mapping it to the selection, which is itself not connected to a tool, but a property of the layer which in turn is visible on the mapcanvas.

  • Use the del key by default to delete features and let a MapTool overwrite this behaviour
  • Always use the del key to delete features and do not allow a MapTool to overwrite this
  • Implement the del key in the MapTools, so its effect changes whenever the MapTool changes (The solution you propsed)
@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 4, 2013

+1 first option, also there are maptools using del-backspace key.

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Dec 4, 2013

+1 for the first option too, I think. To be more precise: use the del key to delete features from current active layer (if in edit mode), or show message (like now) that the layer is not in edit mode...
My argument: see QGIS as a digitizing tool of which the main objects are 'geographic features' and second: QGIS should behave more or less like a vector drawing application (preferably inkscape).

and another feature request: after ctrl-D in the TOC, can it be that 'OK' is the default choice. This makes it a little more friendly for users to hit 'enter' ....

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 4, 2013

Ok, I will fix it!

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 5, 2013

Hi, last commit contains:

  • '<' and '>' to activate previous and next node in 'node tool', for demo proposes, I will use the keys what you want!
  • Assigns OK button as default in confirmation dialog to remove layer[s] in the TOC.
  • Use 'backspace/delete' keys as default shortcut to delete selected features in MapCanvas. Several MapTools override these keys avoiding the default behavior.

About last section, it works, but I would love some guru validate the implementation I've done.
Regards

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Dec 5, 2013

hi @ahuarte47
tested, and all works as you say above.
maybe sent an email to the dev and or user list to test this? Or should we pull and ask people te test in the nightlies?

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 6, 2013

Hi RIchard, I can send a message as I did above to request opinions.
it's okay for you to use '<' and '>' to activate the nodes ?

Alvaro

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Dec 6, 2013

yes, sure. No problem. Please do

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 6, 2013

Thank you very much Richard for your support!

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 9, 2013

Hi @rduivenvoorde, about using the backspace key in python window, I have verified that python console (plugins -> python console) already uses backspace and delete keys.

What window do you say ?

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 9, 2013

Hi Tim, I added 'Backspace' key in other tools to normalize these two keys as 'delete' action.
Thanks!

ahuarte47 added some commits Nov 25, 2013

#9094: Auto-select adjacent last-removed node
Automatically selects the adjacent vertex to last removed selected node
#9094: '+' and '-' keys work as selector nodes
'+' and '-' select next and previous to current active node
respectively.
This commit is for demo proposes.
#9094: keyPressed can be overrided in MapTool
KeyPressed event of MapCanvas can be overrided in current MapTool. It
enables to use 'Delete/Backspace' key as default shortcut to delete
selected features, but it can be overrided in current maptool.
@@ -24,7 +24,7 @@
* If you want to synchronize the attribute table selection with the map canvas selection, you
* should use { @link QgsVectorLayerSelectionManager } instead.
*/
class GUI_EXPORT QgsGenericFeatureSelectionManager : public QgsIFeatureSelectionManager
class CORE_EXPORT QgsGenericFeatureSelectionManager : public QgsIFeatureSelectionManager

This comment has been minimized.

Copy link
@jef-n

jef-n Dec 19, 2013

Member

What's this about?

This comment has been minimized.

Copy link
@ahuarte47

ahuarte47 Dec 19, 2013

Author Contributor

Sorry, it should not be here! I had to change it temporarily for some time to compile.
Can you remove it ?

This comment has been minimized.

Copy link
@ahuarte47

ahuarte47 Dec 19, 2013

Author Contributor

Hi @jef-n, I have fixed the incorrect changes
Thank you very much

@ghost ghost assigned timlinux Dec 30, 2013

@timlinux timlinux merged commit e3e01fc into qgis:master Dec 30, 2013

@timlinux

This comment has been minimized.

Copy link
Member

timlinux commented Dec 30, 2013

I have merged these changes thanks! One other comment: the < > keybindings require pressing shift on my keyboard. Perhaps you could add / change them to , and . So that the mnemonic is the same but not modifier key is needed.?

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 30, 2013

Thanks @timlinux.
About ',' and '.', I can add these keys too. Do you think that they are more suitable than '<' and '>' ?

@timlinux

This comment has been minimized.

Copy link
Member

timlinux commented Dec 31, 2013

Hi

Sent from my mobile phone
On 30 Dec 2013 11:26 PM, "Alvaro Huarte" notifications@github.com wrote:

Thanks @timlinux.
About ',' and '.', I can add these keys too. Do you think that they are
more suitable than '<' and '>' ?

Yes because (on my keyboard anyway) they do not require pressing shift. You
still have the visual queue of the <>keys to remind you which keys to use.

Regards

Tim


Reply to this email directly or view it on GitHub.

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Dec 31, 2013

@timlinux

This comment has been minimized.

Copy link
Member

timlinux commented Dec 31, 2013

Thanks! Will / did you make a separate PR for that?

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Jan 2, 2014

Done:
#1048

Thanks @timlinux

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Jan 7, 2014

@ahuarte47 hi Alvaro, do you think http://hub.qgis.org/issues/9308 is related to this?

@ahuarte47

This comment has been minimized.

Copy link
Contributor Author

ahuarte47 commented Jan 7, 2014

Hi @rduivenvoorde, you are right, it is a bug generated by me in this PR
It will fix it!

Thank you very much!

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