Modify

Opened 14 years ago

Closed 14 years ago

#3475 closed enhancement (fixed)

Add filter/layer support to josm

Reported by: anonymous Owned by: bastiK
Priority: normal Milestone:
Component: Core Version: latest
Keywords: patch Cc: bilbo, PetrDlouhy, jklippel

Description

As OSM is getting more and more complex, it's modification is getting more and more complicate. JOSM should have possibility to filter certain objects out from the map view / prohibit their editation.

I have read about need of layer support in several independent discussions (even that it should have been part of OSM api).

So here it is: I have made patch, which is adding filter support to OSM. Filters are made out of search query, and are very strong tool.

Attachments (10)

diff3 (50.3 KB ) - added by PetrDlouhy 14 years ago.
filters.patch
filters_core.diff (19.9 KB ) - added by PetrDlouhy 14 years ago.
filters_dialogs.diff (29.2 KB ) - added by PetrDlouhy 14 years ago.
filters_dialogs.2.diff (28.9 KB ) - added by PetrDlouhy 14 years ago.
new version without mathematic symbols
improvements.diff (5.2 KB ) - added by PetrDlouhy 14 years ago.
enable_hide.diff (8.6 KB ) - added by PetrDlouhy 14 years ago.
Patch adding enable/hide and counting of filtered elements.
enable_hide1.diff (11.8 KB ) - added by PetrDlouhy 14 years ago.
Same as previous patch + some minor fixes of significant bugs
backRefVisitor.diff (11.1 KB ) - added by PetrDlouhy 14 years ago.
backRefVisitor.2.diff (11.2 KB ) - added by PetrDlouhy 14 years ago.
fixed version - routes plugin is working
backRefVisitor.3.diff (11.2 KB ) - added by PetrDlouhy 14 years ago.
another fix

Download all attachments as: .zip

Change History (57)

by PetrDlouhy, 14 years ago

Attachment: diff3 added

filters.patch

comment:1 by bilbo, 14 years ago

Cc: bilbo added

comment:2 by avarab@…, 14 years ago

Keywords: patch added
Summary: Add filter/layer support to josm[PATCH] Add filter/layer support to josm
Type: defectenhancement
Version: latest

comment:3 by bastiK, 14 years ago

Cool stuff!

Please make it Java 5 compatible, e. g. delete the @Override for interface implementations.

comment:4 by anonymous, 14 years ago

OK, I will do it.
By the way - JOSM code is not compilable (2 errors and 6 warnings) by default (try ant clean).

comment:5 by bastiK, 14 years ago

I tried - compiles on my system (4 warinigs, javac 1.5.0_16, r 2114)

comment:6 by bastiK, 14 years ago

The more i use it, the more i like it :)

Some thoughts:


  • The first column should be called "enabled / disabled". When disabled, the whole line should be grey.


  • The mathematical symbols in the "mode" column are nice, but as most people don't know them i would recommend using letters: (R)eplace, (A)dd, (D)elete, (F)ind


  • I think for the header, one can find nice descriptive icons. (Note that the tools tips don't work there.)


  • Then one should think about some kind of Bookmarks or Filter profiles. The basic idea is that i have a bunch of filters, that could be useful later, but i don't want to see them in the list all along.


  • Please attach the image filter.png as it is not in the diff.


About the dialogue:


  • The help on the right is really nice, but having to type a name first is confusing. The users have to think about how they can filter the stuff they want and shouldn't be bothered to make up a stupid name. Most filter strings would fit in the list anyway.


  • Instead of "Make filter" find some text that is appropriate for later edits of the same filter, i. e. "OK".


I hope you find these comments helpful (and that this gets committed really soon :) ).

comment:7 by Gubaer, 14 years ago

I already tried to commit. The three @Override are easily fixed. But I need the missing filter.png first.

comment:8 by stoecker, 14 years ago

Owner: changed from team to anonymous
Status: newneedinfo

One first impression after a first look over the code:

x.isDeleted()
x.isDisabled() x.incomplete

should be

!x.isUsable() and the disabled check should be added to isUsable().

The changes to use isUsable() can be in an independent patch which can be checked in without problems now.

I expect many problems from this patch, so it has to wait till next latest or must be disabled by default and only disabled on hidden config option.

Can you change the patch accordingly?

P.S. Thanks for the work.

Karl: Please don't commit it as it is.

comment:9 by Gubaer, 14 years ago

Karl: Please don't commit it as it is

That's fine by me. I'll leave this one for you.

comment:10 by PetrDlouhy, 14 years ago

Dirk:

OK, that seems reasonable, I will do it.

Others:

I will think it over, and do something about it. I don't have any icon for the filters nor for other things. Is here somebody more talented than me, who would like to make them?

I think, that in the mode column should also be nice comprehensible icons, because it could be difficult to understand what dependency it has for the filter.

comment:11 by stoecker, 14 years ago

Petr: One side note - From my text above I prefer the "disabled by default" BTW. I really want this in JOSM, but as said I expect problems and want to have a testing phase with the more experienced JOSM users first. The problem with delaying the patch is that it will be out of sync soon and this makes lots of work.

I remember the troubles when I introduced the virtual nodes and want to prevent similarities :-)

comment:12 by PetrDlouhy, 14 years ago

Cc: PetrDlouhy added

OK, here are the divided patches.

by PetrDlouhy, 14 years ago

Attachment: filters_core.diff added

by PetrDlouhy, 14 years ago

Attachment: filters_dialogs.diff added

comment:13 by stoecker, 14 years ago

(In [2120]) see #3475 - patch by Petr Dlouhý - code rework for display filtering

by PetrDlouhy, 14 years ago

Attachment: filters_dialogs.2.diff added

new version without mathematic symbols

comment:14 by avarab@…, 14 years ago

When using attachment:filters_dialogs.2.diff with r2120 I get the following error when downloading data:

java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at org.openstreetmap.josm.gui.PleaseWaitRunnable$2.run(PleaseWaitRunnable.java:98)
	at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:597)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)
Caused by: java.lang.reflect.InvocationTargetException
	at java.awt.EventQueue.invokeAndWait(EventQueue.java:997)
	at org.openstreetmap.josm.gui.PleaseWaitRunnable.doRealRun(PleaseWaitRunnable.java:67)
	at org.openstreetmap.josm.gui.PleaseWaitRunnable.run(PleaseWaitRunnable.java:116)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441)
	at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
	at java.util.concurrent.FutureTask.run(FutureTask.java:138)
	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
	at java.lang.Thread.run(Thread.java:619)
Caused by: java.lang.NullPointerException: /images//dialogs/filter.png not found
	at org.openstreetmap.josm.tools.ImageProvider.get(ImageProvider.java:72)
	at org.openstreetmap.josm.tools.ImageProvider.get(ImageProvider.java:211)
	at org.openstreetmap.josm.actions.JosmAction.<init>(JosmAction.java:69)
	at org.openstreetmap.josm.gui.dialogs.ToggleDialog$ToggleDialogAction.<init>(ToggleDialog.java:58)
	at org.openstreetmap.josm.gui.dialogs.ToggleDialog$ToggleDialogAction.<init>(ToggleDialog.java:56)
	at org.openstreetmap.josm.gui.dialogs.ToggleDialog.init(ToggleDialog.java:135)
	at org.openstreetmap.josm.gui.dialogs.ToggleDialog.<init>(ToggleDialog.java:120)
	at org.openstreetmap.josm.gui.dialogs.FilterDialog.<init>(FilterDialog.java:51)
	at org.openstreetmap.josm.gui.MapFrame.<init>(MapFrame.java:107)
	at org.openstreetmap.josm.Main.addLayer(Main.java:222)
	at org.openstreetmap.josm.actions.downloadtasks.DownloadOsmTask$Task.finish(DownloadOsmTask.java:130)
	at org.openstreetmap.josm.gui.PleaseWaitRunnable$1.run(PleaseWaitRunnable.java:69)
	at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:199)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:597)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:178)
	at java.awt.Dialog$1.run(Dialog.java:1045)
	at java.awt.Dialog$3.run(Dialog.java:1097)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.awt.Dialog.show(Dialog.java:1095)
	at java.awt.Component.show(Component.java:1563)
	at java.awt.Component.setVisible(Component.java:1515)
	at java.awt.Window.setVisible(Window.java:841)
	at java.awt.Dialog.setVisible(Dialog.java:985)
	at org.openstreetmap.josm.gui.progress.PleaseWaitProgressMonitor$4.run(PleaseWaitProgressMonitor.java:102)
	at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:597)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)

It doesn't happen without this patch so I'm pretty sure it's the cause.

in reply to:  14 comment:15 by avarab@…, 14 years ago

Replying to avar:

When using attachment:filters_dialogs.2.diff with r2120 I get the following error when downloading data:
[...]
It doesn't happen without this patch so I'm pretty sure it's the cause.

Nevermind this is just because filter.png is missing.

Please attach it to the bug.

comment:16 by stoecker, 14 years ago

(In [2123]) see #3475 - patch by Petr Dlouhý - display filtering - disabled by default some time until more mature

comment:17 by stoecker, 14 years ago

Hey, I need some time to check code. Don't update your patches so fast :-)

Could you please attach an updated patch - should be easier for you than for me.

comment:18 by stoecker, 14 years ago

One note of first tests: Adding a new filter and startup with filtering enabled is VERY slow even for a small dataset. Much work still to be done, but gladly we are going in that direction for some time now :-)

comment:19 by PetrDlouhy, 14 years ago

It's ok, it's just minor change, I will attach it with bunch of other changes (probably tomorrow).
I think, that something went wrong with applying that patch - you probably foregot to add new files (Filter, Filters and FilterDialog).

comment:20 by xeen, 14 years ago

Hm, seems the filter.java file is missing, causing JOSM's build to fail?

comment:21 by stoecker, 14 years ago

Fixed in r2125. But I did not forget the icon :-)

comment:22 by PetrDlouhy, 14 years ago

Here is patch with some further changes (no mathematic symbols, no name for the filter).

  • The first column should be called "enabled / disabled". When disabled, the whole line should be grey.

:In current version, it is possible to have element filtered but not disabled (I taught, it could be usable sometimes). If you think, that is bad idea, I can do it that way.

by PetrDlouhy, 14 years ago

Attachment: improvements.diff added

comment:23 by framm, 14 years ago

Very intersting. Played around with it for a while. Lots of usability issues (likely that people will get themselves into situations where they have done something wrong and nothing works). Will probably need prominent info somewhere saying "1234 objects total, 235 hidden, 456 filtered...". First impressions:

  • The name "filter" should perhaps be avoided because of the ambiguity: If I filter all restaurants does that mean I am left with only restaurants or with the data minus the restaurants? Maybe we can find better wording. In the dialog we already have "disable elements", and the one next to it should perhaps simply be "hide elements" not "filter elements".
  • I would expect disabled-but-visible elements to be grayed out, including their icons.
  • An object that is currently selected does not react to filter activation (it does not become invisible if I enable a filter that would make it invisible, and remains selected). If I edit an object and give it a property that would make it filtered, it remains visible until deselected.
  • Does it make sense to allow invisible-but-not-disabled objects to be selected? This means you can click into an empty space, assuming to draw a selection rectangle, but instead you are moving a hitherto invisible object!
  • When I make a filter to hide everything tagged "highway:", nodes that support a highway are also hidden (which is good), but if they have a special meaning (e.g. crossing=traffic_signals) they are ALSO hidden (which is bad).
  • When a node is disabled/hidden but part of a way, it can still be moved by moving the whole way. Unsure if that is right, might have unforeseen consequences.
  • Will definitely (in the long run - not for version 1 of course) need a simple way for users to choose from pre-defined filters (or filter chains), things like "address edit mode", "railway edit mode" and the like.
  • Would be good to think about giving a warning if people create new objects very near existing-but-invisible objects.


comment:24 by stoecker, 14 years ago

(In [2129]) see #3475 - minor cleanup

by PetrDlouhy, 14 years ago

Attachment: enable_hide.diff added

Patch adding enable/hide and counting of filtered elements.

comment:25 by bastiK, 14 years ago

Patch adding enable/hide and counting of filtered elements.

Yes, I think it is much less confusing this way

Other things:

Replying to framm:

  • The name "filter" should perhaps be avoided because of the ambiguity: If I filter all restaurants does that mean I am left with only restaurants or with the data minus the restaurants?

How call it instead? It's a filter after all.

The choice what is default behaviour and what is inverted is a little at random, though. What about two radio buttons on the edit dialogue:

x hide filtered elements
o show only filtered elements

Then instead of the inverted column, toggle these modes by left click.

Alternatively explain it somewhere on the dialogue. Underneath this text, there could be a duplicate of the invert check box with some hint what this means in contrast to the default behaviour.


Maybe move "apply also for children" to the edit dialogue and make it more clear what it means. Then there could be something like "only for untagged children" underneath. This would be grey if the first check box is off.


I'm not sure, but all these modes and how they influence each other -- could be a little too complicated for the average user.

  • "Replace selection" makes all preceding filters useless, right? So it might be convenient sometimes but is not indispensable.
  • "Remove from selection": A \ B = A "find in" ("inverted" B).


  • so it boils down to some kind of "AND" and "OR". If only one of them is used, they also commute, so you don't need to know if the filters act from top to bottom or bottom to top.

So why not keep it simple as long as no expressiveness is lost...

Also please reconsider the term "selection", it could be associated with the nodes / ways that are selected by clicking on them.


Replying to stoecker:

[...] VERY slow even for a small dataset.

This will certainly be improved but in the meantime, what about some progress dialogue?

in reply to:  25 comment:26 by PetrDlouhy, 14 years ago

Replying to bastiK:

So why not keep it simple as long as no expressiveness is lost...

Yes, you can replace some modes by other modes and enabling/inverting other filters. But it is not the right question - it could be much too work (or thinking) to make those changes. The right question is: will be some modes really so rare-used, so they can be omitted?

I didn't test the filters on real work yet, so I don't know. Please try to think about cases in which they are useful, if so.

Also please reconsider the term "selection", it could be associated with the nodes / ways that are selected by clicking on them.

Please suggest something better, that could be shared with find dialog.

comment:27 by bastiK, 14 years ago

The point is, it shouldn't be shared with the find dialogue.

With filters there is no longer a current selection that changes interactively with time. Instead the order and the modes can be changed arbitrarily after the filters are created, which might get complicated.

But i agree there are more important things one can do right now than removing features. So i don't mind if it stays like this.

comment:28 by PetrDlouhy, 14 years ago

Hello, now I realized the delay when loading dataset with filters on. It is probably because filters are recalculated with dataChanged(OsmDataLayer l) from DataChangeListener. It is possible to distinguish that it is in loading phase? Or should be data layer modified not to fire dataChanged during loading? Is it possible to find out what change was done (It would probably be better to modify filter's selection with only with new items better than whole data set)?

comment:29 by PetrDlouhy, 14 years ago

There is also another thing. If "Apply also for children" option is enabled, the filters just modify search string. The problem is, that "child" search is VERY slow.
Are there some plans to slower that operation?

by PetrDlouhy, 14 years ago

Attachment: enable_hide1.diff added

Same as previous patch + some minor fixes of significant bugs

comment:30 by stoecker, 14 years ago

(In [2145]) see #3475 - patch by Petr Dlouhý - cleanups

comment:31 by stoecker, 14 years ago

Owner: changed from anonymous to team
Status: needinfonew
Summary: [PATCH] Add filter/layer support to josmAdd filter/layer support to josm

comment:32 by PetrDlouhy, 14 years ago

Hello again,

I remade CollectBackReferencesVisitor so now it is visiting with O(1) and only constructor has O(n) instead of every visiting call being O(n). With that child search is quick enough.

The interface of that class had to be changed a little - it should be instantiated outside of the cycle, and in cycle should be called initialize() instead of constructor. I made those changes for JOSM, but it could be contained also in some plugins (e.g. routes).

by PetrDlouhy, 14 years ago

Attachment: backRefVisitor.diff added

by PetrDlouhy, 14 years ago

Attachment: backRefVisitor.2.diff added

fixed version - routes plugin is working

by PetrDlouhy, 14 years ago

Attachment: backRefVisitor.3.diff added

another fix

comment:33 by Gubaer, 14 years ago

Summary: Add filter/layer support to josm[Patch] Add filter/layer support to josm

comment:34 by stoecker, 14 years ago

(In [2166]) see #3475 - patch by Petr Dlouhý - improve speed

comment:35 by stoecker, 14 years ago

Summary: [Patch] Add filter/layer support to josmAdd filter/layer support to josm

comment:36 by Gubaer, 14 years ago

Ticket #2288 has been marked as a duplicate of this ticket.

comment:37 by jklippel, 14 years ago

Cc: jklippel added

comment:38 by anonymous, 14 years ago

Hello,

maybe the function could be activated by default now when new tested was released. I could be deactivated again for next tested, If some problems show up.

comment:39 by anonymous, 14 years ago

I -> It

comment:40 by anonymous, 14 years ago

It is still a long way until this can be presented default.

comment:41 by PetrDlouhy, 14 years ago

Please explain.

It could be only for latest releases, and for tested it can be disabled again.

I think that will increase number of testers, so more comments will show up. I don't understand, why to hide this feature - it is not very user friendly, but it is very useful yet.

The unimplemented usability requests are those from Frederik. OK, that's thing to work on. But will the feature be enabled afterwards?

comment:42 by stoecker, 14 years ago

To many people use latest all-day. We cannot risk larger numbers of corrupted data due to misunderstandings in the way the filter works. It must be bullet-proof before we can make such fundamental changes default.

comment:43 by PetrDlouhy, 14 years ago

Ok. You are right.

If we want to make the function bulletproof we need to say, what are the weaknesses.

So please (you all) write what needs to be improved to make it good enough (except what Frederik wrote).

comment:44 by stoecker, 14 years ago

The most important issue is: For a normal educated user (i.e. no JOSM or OSM specialist) usage of that function should not result in loosing data or duplicating data are wrongly connections, ... This also includes the plugins.

The software state must be consistent. There should not be any situations where JOSM suggests to do XY and you are not able to do this.

The speed problem is also a major issue.

comment:45 by anonymous, 14 years ago

Wasn't the speed problem resolved in my latest patch? I don't see any major speed degression now. If you do, please write, how it can be reproduced.

comment:46 by bastiK, 14 years ago

Owner: changed from team to bastiK
Status: newassigned

I'm doing a new gui for this.

comment:47 by stoecker, 14 years ago

Resolution: fixed
Status: assignedclosed

See [3189].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain bastiK.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.