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

improve DistanceTool to work correctly during Map zoom in and zoom out actions #333

Merged
merged 3 commits into from Feb 22, 2019

Conversation

nprigour
Copy link
Contributor

When pwrforming a zoom in or zoom out while using the DistanceTool the DistanceTool lines do not scale correctly (actually they do not scale at all) together with the map leading to weird results. This PR attempts to fix this by storing clicked points as coordinates and not points and performing the conversion during final rendering on screen.
Another enhancement introduced by this PR is that the displayed information at the bottom left of the screen includes also information about the last segment length.

Signed-off-by: Nikolaos Pringouris nprigour@gmail.com

actions and also display last segment length

Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
@fgdrf
Copy link
Contributor

fgdrf commented Feb 19, 2019

Great, thanks for contributing. I'll test it and give you feedback. In between I triggered Jenkins to give feedback first : https://ci.eclipse.org/udig/job/uDig-PR/39/

@fgdrf fgdrf added this to the 2.1.0 milestone Feb 19, 2019
@nprigour
Copy link
Contributor Author

nprigour commented Feb 19, 2019

The jenkins failure is irrelevant to the present PR. However I think we had this kind of failure in the past around one year ago and somehow it was resolved. Now it seems to appear again. Strange things! Did we change anything?

@@ -1,2 +1,2 @@
DistanceTool_distance = Entfernung \= {0}
DistanceTool_distance = Entfernung \= {0} (letztes segment \= {1})
Copy link
Contributor

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 "Gesamtstrecke = {0} (aktuelles Segment = {1})"

Copy link
Contributor Author

@nprigour nprigour Feb 19, 2019

Choose a reason for hiding this comment

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

OK modification commited after all you are the German guy

@fgdrf
Copy link
Contributor

fgdrf commented Feb 19, 2019

checked current docs and I guess the page is still valid : http://udig.github.io/docs/user/reference/Information%20Tools#distance-tool

Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
Copy link
Contributor

@fgdrf fgdrf left a comment

Choose a reason for hiding this comment

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

Then I’d like to suggest to remove all files for languages we have no native speakers. So let’s keep English and German for now and let’s translation community at Transifex to contribute..

@nprigour
Copy link
Contributor Author

nprigour commented Feb 19, 2019

For the italian translation from the little I know we are OK.
Now about the rest translations, the initial versions of the message files contained an entry for the DistanceTool_distance property which I just augment it with the string after the 1st argument. If I restore it as it was before then we may have a problem since the MessageFormat.format(...) call in line 247 expects 2 extra arguments. Unless we decide to replace the string part within the parenthesis with the english translation?

@fgdrf
Copy link
Contributor

fgdrf commented Feb 20, 2019

I'd prefer to remove properties files we not totally sure about translation. Would you remove all oher message files than messages.properties, messages_de.properties, and messages_it.properties (if you are sure about)?

@fgdrf
Copy link
Contributor

fgdrf commented Feb 20, 2019

I'd like to suggest a little optimization. What do you think about switching the display of the last segement and total distance such as "Distance: {0} (total: {1})" and displaying the information directly at the cursor rather than in the Status bar:

here is a little example:

grafik

I can create a pull request for your branch...

@nprigour
Copy link
Contributor Author

nprigour commented Feb 20, 2019

Why not have them both? In any case you can proceed with the suggested PR in order to check behavior .

By the way will you provide an indication of length for each segment as seen on map?

@fgdrf
Copy link
Contributor

fgdrf commented Feb 21, 2019

Why not have them both?

Do you mean having a rendered label at cursur in Map and updted statusbar?

In any case you can proceed with the suggested PR in order to check behavior

✔️ Done, its fine from my perspective and ready to merge except i18n ressources we should remove from this pull ;)

By the way will you provide an indication of length for each segment as seen on map?

Yeah, forgot to mentioned it, every segment could get rendered a label as shown. However it might have conflicts to other labels of other segments. Could be optional ..

@nprigour
Copy link
Contributor Author

nprigour commented Feb 21, 2019

Hi @fgdrf ,
Yes we can keep the status bar info and you can also add the information on the map as you suggest in the screenshot.

Now I thought you will provide a PR on my existing PR branch. Isn't that the case? Do you want to proceed first with merging the present and you create a separate PR on the master branch?. If yes, please confirm and I will proceed with the removal of the i18n resources i order to conclude the PR.

Now regarding the labelling in each segment indeed it may cause some clutter but in many cases will be very useful. May be you can control it by an appropriate preference option?

@fgdrf
Copy link
Contributor

fgdrf commented Feb 21, 2019

@nprigour Sorry for confusion, lets proceed with this PR as it is and I'll create another with minor optimizations. It takes a bit longer :P

For now, lets proceed with the removal of the i18n resources and we can merge this PR! Thanks in advance for fixing it!

Preferences Option is a good point I'll address!

Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
@nprigour
Copy link
Contributor Author

OK remove i18n for es, eu, ru, ko country codes

@fgdrf fgdrf merged commit a747cde into locationtech:master Feb 22, 2019
fgdrf pushed a commit to fgdrf/udig-platform that referenced this pull request Dec 23, 2019
…t actions (locationtech#333)

* improve DistanceTool to work correctly during Map zoom in and zoom out
actions and also display last segment length

Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants