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

Do not display passwords of GeoResources and Services in UI #262

Merged
merged 7 commits into from
Feb 12, 2018

Conversation

nprigour
Copy link
Contributor

@nprigour nprigour commented Jan 7, 2018

Usually we do not want passwords to be displayed in the UI (i.e. for catalog servives or layer resources corresponding to jdbc resources).
This pull request provides some extra methods in IGeoResource, IService and LayerImpl that can be used for displaying the ID value without displaying the password.

Signed-off-by: Nikolaos Pringouris [email protected]

@nprigour
Copy link
Contributor Author

nprigour commented Jan 7, 2018

Example screenshots provided:

  • Layer Resource
    default

  • Catalog View
    default

* @param layer
* @return
*/
public static String getDisplayID(final Layer layer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering why the method signiture has not been added to Interface org.locationtech.udig.project.internal.Layer
Since the interface is intenal, nobody else should have implemented another layer .. but could be because package org.locationtech.udig.project.internal is exported .. hmm

Any opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably it could be also have been implemented the way you mention.
I think I choose to do it this way (static method) due to the fact that Layer inherits from EObject and I did not want to give the impression that this is a member returning function.
If you think it is more clear the way you propose we can change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nprigour I had a look at the changset today. I'd like to understand two methods in LayerImpl and IGeoResource which looks like a bit duplicated code to me. Is there any reason why LayerSummary cannot add layer.getGeoResource().getDisplayID()?

The second note I have : Have you used the codeformatter.xml configuration to format new code? If not, please have a look at https://github.com/locationtech/udig-platform#preferences and format new code blocks you contributed. It helps us to read the code regarding tabs and spaces ;)

Many thanks

@nprigour
Copy link
Contributor Author

Hi @fgdrf ,

I have modified the code to LayerSummary in order to use IGeoResource DisplayID() method and remove it from LayerImpl. Also fix formatting using the udig formatter (sorry for the formatting issue but since I am also cpontributing to geotools and working to my own project on the same eclipse IDE, sometimes I forgot to change the used formatter!!)

@fgdrf
Copy link
Contributor

fgdrf commented Feb 11, 2018

I'm going to test rebasing to master and if no test failing I'm going to rebase and merge it. Thanks again for providing this improvement!

@fgdrf
Copy link
Contributor

fgdrf commented Feb 11, 2018

Hi @nprigour,
Looks like everything works fine (at least on my machine). Please see pull request #6 your branch that addes a test case and rebased commits to latest improvements on master (where almost all tests are successful).

@fgdrf fgdrf added this to the uDig-2.0.0 milestone Feb 11, 2018
@nprigour
Copy link
Contributor Author

Hi @fgdrf`,
I had a look at it and think it is pretty OK.

@nprigour
Copy link
Contributor Author

I also provide a small correction in LayerSummary that I introduce by inadvertence in my previous commit

@fgdrf
Copy link
Contributor

fgdrf commented Feb 12, 2018

@nprigour
Copy link
Contributor Author

nprigour commented Feb 12, 2018

Ηι @fgdrf

Is the 1 failure that jenkins CI produces related to this pull request? It does not seem very relevant to me

@fgdrf
Copy link
Contributor

fgdrf commented Feb 12, 2018 via email

@fgdrf fgdrf merged commit 5be53a6 into locationtech:master Feb 12, 2018
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.

2 participants