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

wxgui: startup GUI automatic detection of grassdata: make case independent #644 #664

Conversation

lindakarlovska
Copy link
Contributor

@lindakarlovska lindakarlovska commented May 23, 2020

After starting GRASS GIS with GUI, the GUI now searches for existing grassdata (e.g., dac6d4a and #644 with #664).

This platfrom-dependent directory would be:

- $HOME (os.path.expanduser('~')) on Linux and the like,
- User's Documents on Windows (see dac6d4a for code trying to identify that dir),
- One of the above on macOS - macOS users, please share your ideas.

This function does not address temporary directory. No platform specific code.

@petrasovaa petrasovaa added gsoc Reserved for Google Summer of Code student(s) GUI wxGUI related labels May 30, 2020
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

We talked about more options there, but some are easy to add in the future to this code and some would require much more work, so I'm fine with this as it is once the style is fixed.

break # get the first match
return path

options = ["grassdata","GRASSDATA","Grassdata","GrassData"]
Copy link
Member

Choose a reason for hiding this comment

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

Please use PEP8 for formatting. In general, you can run Flake8 (flake8 startup/utils.py) to get some errors, but some are unfortunately still ignored/disabled including this one. Here, just remember space after comma and use: ..."grassdata", "GRASSDATA"....

@mlennert
Copy link
Contributor

mlennert commented Jun 6, 2020

Wouldn't it be easier to just check whether the string transformed to all minor case is equal to 'grassdata' ?

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jun 6, 2020

Wouldn't it be easier to just check whether the string transformed to all minor case is equal to 'grassdata' ?

I thought about it too, but the problem is that I don't have any string that I can transform. I have no idea what combination of lowercase and uppercase letters in the "grassdata" a user could have on his computer.

@neteler
Copy link
Member

neteler commented Jun 6, 2020

I thought about it too, but the problem is that I don't have any string that I can transform. I have no idea what combination of lowercase and uppercase letters in the "grassdata" a user could have on his computer.

Maybe just convert all to lowercase?

https://grass.osgeo.org/programming7/strings_8c.html
--> G_str_to_lower (char *str)

It seems to be wrapped:
ag --python G_str_to_lower lib/python/ctypes/OBJ.x86_64-pc-linux-gnu/gis.py 
3834:if hasattr(_libs['grass_gis.7.9'], 'G_str_to_lower'):
3835:    G_str_to_lower = _libs['grass_gis.7.9'].G_str_to_lower
3836:    G_str_to_lower.argtypes = [String]
3837:    G_str_to_lower.restype = None

@mlennert
Copy link
Contributor

mlennert commented Jun 6, 2020

Wouldn't it be easier to just check whether the string transformed to all minor case is equal to 'grassdata' ?

I thought about it too, but the problem is that I don't have any string that I can transform. I have no idea what combination of lowercase and uppercase letters in the "grassdata" a user could have on his computer.

I understand. You would have to approach it differently, i.e. get the list of all directories and then identify those that contain 'grassdata'. Something like this:

import os
for candidate in candidates:
    for dir in next(os.walk(candidate))[1]:
        if 'grassdata' in dir.lower():
            print(dir)

Obviously something else than print() is needed...

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

The new functionality needs to be a new function. They are related, but the new directory should be created only if none of the paths is found. So, two functions total.

@lindakarlovska lindakarlovska marked this pull request as draft June 10, 2020 10:23
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Sorry, I should have been more clear before: There are independent enough not only to be two functions, but also two different PRs. I think both can be just branched out from master. Two PRs make it easier to review and would also correspond with the two separate issues with distinct goals this is addressing.

…for the directory named grassdata in usual locations.
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

I don't think the behavior should be platform-dependent. The point is to find an existing directory. Please, implement this according to #644 as we originally discussed, i.e., make it case independent one way or the other and that's it. Don't mix it with #682 which is about what happens after no directory is found.

The platform-specific code just makes this more complicated than needed and you would anyway have to handle cases such as Linux user has grassdata in Documents or Windows user has it in "home".

The goal of this code is to find the directory wherever it is. Just make this same for all platforms. The only cases where this will give sub-optimal result is when user has multiple grassdata directories, but there is no right answer for that anyway and it is not really the use case this is trying to address.

With that said, we can reconsider how this code should behave in light of #682, but that's again not really relevant to #644.

@hellik
Copy link
Member

hellik commented Jun 14, 2020

See e.g.

https://www.itprotoday.com/cloud-computing/what-environment-variables-are-available-windows

There is a environment variable in MS Windows to get the user's home in an easy way.

@wenzeslaus
Copy link
Member

@hellik What's wrong with os.path.expanduser?

On Unix and Windows, return the argument with an initial component of ~ or ~user replaced by that user’s home directory.

On Windows, USERPROFILE will be used if set, otherwise a combination of HOMEPATH and HOMEDRIVE will be used. An initial ~user is handled by stripping the last directory component from the created user path derived above.

Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

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

Please also clean up extra whitespace.

try:
# here goes everything which has potential unicode issues
candidates.append(os.path.join(home, _("Documents"), "grassdata"))
candidates.append(os.path.join(home, _("My Documents"), "grassdata"))
candidates.append(os.path.join(home, _("Documents")))
Copy link
Contributor

Choose a reason for hiding this comment

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

This adds Documents into the list of candidates twice in case _("Documents") == "Documents". So you could do:

tr_documents = os.path.join(home, _("Documents"))
if tr_documents not in candidates:
   candidates.append(tr_documents)

for subdir in next(os.walk(candidate))[1]:
if 'grassdata' in subdir.lower():
path = os.path.join(candidate, subdir)
break
Copy link
Contributor

Choose a reason for hiding this comment

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

break breaks only the inner loop, you need to return the path right away:

    for candidate in candidates:
        if os.path.exists(candidate):
            for subdir in next(os.walk(candidate))[1]:
                if 'grassdata' in subdir.lower():
                    return os.path.join(candidate, subdir)
    return None

then you don't need to define path variable at all.

gui/wxpython/startup/utils.py Outdated Show resolved Hide resolved
gui/wxpython/startup/utils.py Outdated Show resolved Hide resolved
gui/wxpython/startup/utils.py Outdated Show resolved Hide resolved
gui/wxpython/startup/utils.py Outdated Show resolved Hide resolved
gui/wxpython/startup/utils.py Outdated Show resolved Hide resolved
@hellik
Copy link
Member

hellik commented Jun 18, 2020

I put "My Documents" there because based on what I found at that time it seemed that it is a possible option (that there will be no "Documents" but only "My Documents"). This is does not seem to be the case for most recent Windows, but might be for some which are in use. I simply don't know, so I would prefer to keep "My Documents" there now, although I would like to see some analysis of this and a better solution in the (possibly distant) future.

AFAIK my documents and all other variants, at least in more recent MS Windows versions, are just some kind of an alias for documents. Thus, you should be safe to use documents.

@ninsbl
Copy link
Member

ninsbl commented Jun 18, 2020

AFAIK my documents and all other variants, at least in more recent MS Windows versions, are just some kind of an alias for documents. Thus, you should be safe to use documents.

Yes, that is the case here as well. What makes it far more worse however, is that - at least in my organisation - Windows home directories are linked to a Microsoft Sharepoint/OneDrive site (kind of google drive, dropbox, ...). There users will most likely experience more or less significant performance penalties because of syncing with online storage...

@mlennert
Copy link
Contributor

Could you be a bit more explicit in your commit messages ? 'Improvements' and 'Improvements according to Vaclav' are just not very easy to interpret to get a quick info about the contents of the commit. ;-)

@lindakarlovska
Copy link
Contributor Author

Could you be a bit more explicit in your commit messages ? 'Improvements' and 'Improvements according to Vaclav' are just not very easy to interpret to get a quick info about the contents of the commit. ;-)

Yes Moritz, I will be. Those commits are set up from several very small changes so I was not sure how to describe it. I will try to be more explicit next time.

@wenzeslaus
Copy link
Member

AFAIK my documents and all other variants, at least in more recent MS Windows versions, are just some kind of an alias for documents. Thus, you should be safe to use documents.

OK, let's than ignore any old or strange Windows and ignore also some Linux distros which had/has only the translated versions (based on my initial research). If we decide we need something like this in the future, we will probably need something more sophisticated anyway (there are Python packages and command line tools to deal with this properly).

@lindakladivova Please, remove all the _("...") candidates, this should simplify your code significantly.

@wenzeslaus
Copy link
Member

... What makes it far more worse however, is that - at least in my organisation - Windows home directories are linked to a Microsoft Sharepoint/OneDrive site (kind of google drive, dropbox, ...). There users will most likely experience more or less significant performance penalties because of syncing with online storage...

@ninsbl Are you talking about using grassdata placed there or even just searching through that directory (one level only, two directories)?

I don't know what GRASS could do here. If you have a slow system or a bad setup, GRASS will be slow.

try:
# here goes everything which has potential unicode issues
candidates.append(tr_document)
except UnicodeDecodeError:
Copy link
Contributor

Choose a reason for hiding this comment

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

append function does not generate UnicodeDecodeError, that was there for os.path.join in case of some issues with translatable strings. Anyway, you don't need to fix it, we decided to completely remove the translatable versions (Vashek will comment on that).


Returns the path as a string or None if nothing was found, so the
return value can be used to test if the directory was found.
"""Finds the directory for possible GRASS Database.
Copy link
Contributor

Choose a reason for hiding this comment

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

The description is awkward. I suggest:

"""Looks for directory 'grassdata' (case-insensitive)
in standard locations to detect existing GRASS Database.
Returns the path as a string or None if nothing was found."""

gui/wxpython/startup/utils.py Outdated Show resolved Hide resolved
gui/wxpython/startup/utils.py Outdated Show resolved Hide resolved
gui/wxpython/startup/utils.py Outdated Show resolved Hide resolved
gui/wxpython/startup/utils.py Outdated Show resolved Hide resolved
gui/wxpython/startup/utils.py Outdated Show resolved Hide resolved
@petrasovaa petrasovaa marked this pull request as ready for review June 20, 2020 18:00
@petrasovaa petrasovaa merged commit 80a7b3f into OSGeo:master Jun 20, 2020
@neteler neteler added this to the 8.0.0 milestone Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Reserved for Google Summer of Code student(s) GUI wxGUI related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants