Add option to create a new note everywhere #732

ENT8R · 2017-12-19T18:06:08Z

This PR adds the ability to create a new note everywhere on the whole earth 🌎
You can either long-press on the map or select it in the menu (Which one do you prefer?) and then a marker will be displayed on the map if the zoom is higher than 15. The next step is to set the exact position of the note and click on ✔️ check if you're done to put in the text of the note. After that, the note will be stored in the database and uploaded the next time you're connected to the internet.
And this is how it looks like:

I hope you like it. 😄
BTW: this PR fixes #47

rugk · 2017-12-20T18:05:37Z

You can either long-press on the map or select it in the menu (Which one do youpref prefer?)

Not a bad idea to offer both possibilities, too.
When choosing the long-press mode, the "selector screen" afterwards could be skipped, as the user likely already tapped at the position where they want to have the mote.

Also, IMHO, the checkmark should not be displayed in the title bar. (Buttons/things there are usually valid for the complete StreetComplete application) Maybe rather choose a more obvious position, such as at the bottom right – you could also hide the zoom/GPS buttons there – when selecting the note position, they are not really needed.

westnordost · 2017-12-28T15:09:49Z

I was just using StreetComplete in Bangkok and really missed rhis function!

👍 for taking the initiative here, who knows how much later this would have been implemented by me. Tomorrow is another train ride ahead, so if I have connection, I will roughly look through it.
Especially the adding-photos function makes this a killer feature.

A few comments about what I can see from your screenshots:

and then a marker will be displayed on the map if the zoom is higher than 15

If that limitation is going to stay, then it should not be possible to create a note from a lower zoom level. I guess a similar functionality as the main openstreetmap.org website would be ok (do they limit it?)
Additionally, I don't like the extra step of placing/moving the note. If when pressing the button, a panel (like for quests) comes up from the bottom and directly offers to input a note, the user can at the same time adjust the note's position by moving the map. Also, the input size is bigger.
I think the description for the create new note screen should be different (and shorter). It should mention that you can select the position of the note by moving the note and otherwise perhaps keep the description similar to what is written on openstreetmap.org when creating a new note.

westnordost

Overall very good code quality! 👍

You need to move that code from the quest-related code however.

westnordost · 2017-12-29T05:19:22Z

app/src/main/java/de/westnordost/streetcomplete/MainActivity.java

+			f.setArguments(args);
+
+			FragmentTransaction ft = getSupportFragmentManager().beginTransaction();
+			ft.add(R.id.map_create_note_container, f, CREATE_NOTE);


Couldn't the normal bottom sheet container be used? (Not sure right now, just looking at the diff)

westnordost · 2017-12-29T05:22:34Z

app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/CreateNoteDao.java

-		values.put(CreateNoteTable.Columns.QUEST_TITLE, note.questTitle);
+		if (note.questTitle != null)
+		{
+			values.put(CreateNoteTable.Columns.QUEST_TITLE, note.questTitle);


Does the table allow NULL there?

Also, could questTitle being null cause a NPE someplace else?

Ok nevermind this comment.

Whoops, I meant to write that someplace else, do mind that comment!

(BTW: You can delete comments…)

Does the table allow NULL there?

I think so... In the StreetCompleteOpenHelper it does not say NOT NULL

Also, could questTitle being null cause a NPE someplace else?

I don't think so... As far as I have seen, there are null-checks everywhere.

westnordost · 2017-12-29T05:24:40Z

app/src/main/java/de/westnordost/streetcomplete/MainActivity.java

+			Bundle args = new Bundle();
+			LatLon pos = mapFragment.getPosition();
+			args.putDouble(CreateNoteDialog.ARG_LAT, pos.getLatitude());
+			args.putDouble(CreateNoteDialog.ARG_LON, pos.getLongitude());


You use CreateNoteFragment but use the constants from CreateNoteDialog. That looks fishy.

Nevermind that comment (can't remove it on mobile)

westnordost · 2017-12-29T05:27:13Z

app/src/main/java/de/westnordost/streetcomplete/quests/OsmQuestAnswerListener.java

@@ -17,4 +18,7 @@

 	/** Called when the user chose to skip the quest */
 	void onSkippedQuest(long questId, QuestGroup group);
+
+	/** Called when the user wants to leave a note which is not related to a quest */
+	void onLeaveNote(String note, ArrayList<String> imagePaths, LatLon position);


Hmm, as the callback doesn't have anything to do with quests (or answering them), the method should be in an own interface and not in this.

westnordost · 2017-12-29T05:29:25Z

app/src/main/java/de/westnordost/streetcomplete/tangram/QuestsMapFragment.java

+	{
+		return controller.getZoom();
+	}
+


Should perhaps go into MapsFragment as it has nothing to do with quests?

westnordost · 2017-12-29T05:31:03Z

app/src/main/java/de/westnordost/streetcomplete/quests/QuestAnswerComponent.java

+	{
+		callbackListener.onLeaveNote(text, imagePaths, position);
+	}
+


Should also not be here. Has nothing to do with quests.

ENT8R · 2017-12-30T17:56:42Z

If that limitation is going to stay, then it should not be possible to create a note from a lower zoom level.

There is already a restriction. If the zoom level is lower than 15, a toast shows up which will ask you to zoom in further because you can't create a note otherwise.

I guess a similar functionality as the main openstreetmap.org website would be ok (do they limit it?)

They are limiting it too: You can only create a new note at a zoom level of 12 or higher.

If when pressing the button, a panel (like for quests) comes up from the bottom and directly offers to input a note

I like that 👍 I will try to implement this.

…from quest-related things

…plete, small optimizations

ENT8R · 2018-01-05T20:55:10Z

With the latest commit, the note creation dialog looks like this:

Is this OK?

The dialog is now loaded into the bottom sheet fragment. The user can also move the map while the note is created. And I followed your proposals to differenciate between the quest-related things and the note creation.

westnordost · 2018-01-07T18:26:08Z

app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/CreateNoteUpload.java

 			}
 		}
-		return note.text;
+		return "via "+ ApplicationConstants.USER_AGENT+":\n\n" + note.text;


Why is this relevant?

I just thought it might be useful if somebody wants to review notes created by StreetComplete with https://ent8r.github.io/NotesReview/
But it is totally fine if you think that this should not be added

westnordost · 2018-01-07T19:36:12Z

Okay, thank you. I will take over and refactor some things before I merge it.

# Conflicts: # app/src/main/java/de/westnordost/streetcomplete/MainActivity.java # app/src/main/res/menu/menu_main.xml # app/src/main/res/values/strings.xml

westnordost · 2018-01-14T11:48:06Z

Refactored to have a common superclass for create note fragment and quest details fragment and many other things.

One thing that definitely needs correction is that there is no feedback whatsoever after one created a note. I think the "unsynced quest counter" should also display the to-be-uploaded notes, even if they don't come out as "stars" in the "answered quests counter".

…Counter counts note changes as well

westnordost · 2018-01-14T14:19:14Z

Okay, I'm done.

Anyone wants to review this again?

Add option to create a new note everywhere

cfd27b2

westnordost requested changes Dec 29, 2017

View reviewed changes

ENT8R added 2 commits December 31, 2017 14:17

Make the creation of a new note shorter and easier and seperate code …

96ee014

…from quest-related things

Append text to note to clarify that the note was created by StreetCom…

e72c71e

…plete, small optimizations

westnordost reviewed Jan 7, 2018

View reviewed changes

westnordost self-assigned this Jan 7, 2018

westnordost added 4 commits January 7, 2018 21:58

wip

26894fe

Merge branch 'master' into notes

64a6f92

# Conflicts: # app/src/main/java/de/westnordost/streetcomplete/MainActivity.java # app/src/main/res/menu/menu_main.xml # app/src/main/res/values/strings.xml

refactor and finalize create notes feature

d1fb829

Merge branch 'master' into notes

b389ea5

Refactor the changes counter to be proper components, UnsyncedChanges…

d473099

…Counter counts note changes as well

westnordost approved these changes Jan 14, 2018

View reviewed changes

westnordost removed their assignment Jan 14, 2018

Merge branch 'master' into notes

430b82e

westnordost merged commit b38c108 into streetcomplete:master Jan 17, 2018

ENT8R deleted the notes branch January 17, 2018 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to create a new note everywhere #732

Add option to create a new note everywhere #732

ENT8R commented Dec 19, 2017 •

edited

rugk commented Dec 20, 2017

westnordost commented Dec 28, 2017

westnordost left a comment

westnordost Dec 29, 2017

westnordost Dec 29, 2017

westnordost Dec 29, 2017

westnordost Dec 29, 2017

westnordost Dec 29, 2017

rugk Dec 31, 2017

ENT8R Dec 31, 2017

westnordost Dec 29, 2017

westnordost Dec 29, 2017

westnordost Dec 29, 2017

westnordost Dec 29, 2017

westnordost Dec 29, 2017

ENT8R commented Dec 30, 2017

ENT8R commented Jan 5, 2018

westnordost Jan 7, 2018

ENT8R Jan 7, 2018

westnordost commented Jan 7, 2018

westnordost commented Jan 14, 2018

westnordost commented Jan 14, 2018

Add option to create a new note everywhere #732

Add option to create a new note everywhere #732

Conversation

ENT8R commented Dec 19, 2017 • edited

rugk commented Dec 20, 2017

westnordost commented Dec 28, 2017

westnordost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ENT8R commented Dec 30, 2017

ENT8R commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westnordost commented Jan 7, 2018

westnordost commented Jan 14, 2018

westnordost commented Jan 14, 2018

ENT8R commented Dec 19, 2017 •

edited