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

Add support to define custom TagUI installation folder #207

Merged
merged 10 commits into from May 23, 2021

Conversation

fi-do
Copy link
Contributor

@fi-do fi-do commented Feb 5, 2021

Hello kensoh,

thanks for this great library. I like it a lot and it works like a charm to automate my stuff.
I had a idea for a feature, maybe it can help to improve your library.

Idea

When I first was playing around with the library, I was wondering where tagui will be installed. After some runs I found the default home path for my system. I started to tinker around how to customize it. To be for example fhs compliant or help users with a space in their username.(#198)

I tinkered the parameter installation_dir to the init function in order to set the absolute installation path. So if someone do not want to use the default path, they would be able to change it.

Example

Change installation target to /home/fi-do/opt/tagui instead of /home/fi-do/.tagui:
r.init(installation_dir="/home/fi-do/opt/tagui)
Pack with custom installation path:
r.pack(installation_dir="/home/fi-do/opt/tagui")
Update with custom installation path:
python update.py "/home/fi-do/opt/tagui"

All the best,
fi-do

Copy link

@Ashwani001 Ashwani001 left a comment

Choose a reason for hiding this comment

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

I like the idea of allowing users to use a different path. I am wondering if it would be better done so using a config file. If config is not set, then home directory can be assumed.

tagui.py Outdated
@@ -458,12 +466,21 @@ def init(visual_automation = False, chrome_browser = True):
else:
tagui_directory = os.path.expanduser('~') + '/' + '.tagui'

# check if custom installation path is set
if installation_dir:
if "tagui" in installation_dir:

Choose a reason for hiding this comment

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

Here it is expected that installation_dir contains tagui folder, but then installation_dir gets passed to setup function where .tagui is getting appended again.

tagui.py Outdated
@@ -308,6 +312,10 @@ def setup():
else:
tagui_directory = home_directory + '/' + '.tagui'

# override with custom dir
if installation_dir:

Choose a reason for hiding this comment

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

Since home_directory = installation_dir is set above, this if block is not needed. The if else right above will take care of adding the appropriate name based on OS. Do note my comment below, it seems that the expectation of installation_dir is different in setup and init.

tagui.py Outdated
print('[RPA][ERROR] - Please use path with target directory name tagui.')
sys.exit(1)
else:
print('[RPA][ERROR] - Your target destination is not a valid path.')

Choose a reason for hiding this comment

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

Why do we not exit in this case?

@fi-do fi-do changed the title Feature/installation path [WIP]Feature/installation path Feb 12, 2021
@fi-do
Copy link
Contributor Author

fi-do commented Feb 12, 2021

Hola Ashwani001,
thanks for your reply. I implemented your feedback.

I think how to deal with the config is a design decision. Personally, I wanted to add the feature with a small footprint. Say if someone initializes rpa with the "installation_dir", he knows what he is doing. And if it was not initialized with it, it runs as usually. But feel free to add that. I'm curious how @kensoh sees this.

@Ashwani001
Copy link

Hey @fi-do
Yup im waiting to hear from @kensoh as well.
Generally it is good to discuss the feature before creating a PR, but thanks for working on it, I think many people can benefit from it.

@kensoh
Copy link
Member

kensoh commented Mar 27, 2021

Hi @fi-do Dominik, thank you so much for being a Hero and helping so many users with their issues!! I was basically missing in action since May last year. Mum severely ill and I took many months of unpaid leave to deal with hospital admin and doing all I can to aid her recovery with alternative natural therapies.

Since Nov I've gone back to work full-time at AI Singapore, but super busy chasing work deliverables and also taking care of my 2-year-old. At AI Singapore, I work on the TagUI project, which RPA for Python is based on a fork of that. I created TagUI in 2016-2017, and continue further development at AI Singapore (government-funded programme to accelerate AI in SG).

PS - baby crying i go manage and continue typing later

@kensoh
Copy link
Member

kensoh commented Mar 27, 2021

Thanks @fi-do and @Ashwani001 for your contribution and discussion! Indeed, there's no doubt having the option to pick TagUI installation directory is a need. For some users, without this option, the package may not even be usable. For eg, user folder in a directory with spaces (there's an open issue at TagUI to revisit if there can be a permanent solution).

For implementation, when I next get some time, will chew over it. What's likely to happen is some modifications to Dominik's implementation and then merging back to master.

One part of putting it as part of the parameters is init() now takes 2 parameters, so users can do r.init(True, False) to mean something. I suspect there would be a more common parameter 3 yet to be discovered. By creating parameter 3 as installation directory now, it means in the future when that common parameter that impacts a broader userbase appears, it would be parameter 4 to maintain backward compatibility.

That means users would be awkward doing r.init(True, False, None, Parameter4). I know this level of thinking may be too obsessively focused on the details, but keeping a simple quick API is one of the goals for this project. Including things like r.click('web_identifier'), r.click('abc.png') and r.click(x, y) all works. This is not the usual way to define functions and typically would be better off as 3 different functions.

From programming and architecture point of view this is bad design. But from usability point of view, user only needs to refer to 1 function and use 1 function to handle 3 different scenarios. So that's was chosen instead of having 3 functions for each function that interacts with the UI. It'll quickly go out of hand and be bloated.

I'll chew over this and revert. I expect this PR to be the first external commit to the package and look forward to more interesting contributions from you @fi-do!

@fi-do
Copy link
Contributor Author

fi-do commented Mar 29, 2021

Hey @kensoh,
thanks for you feedback. I still learning Python but I am happy to help out.
I understand your point of view to carefully decide about the signature of functions.

When checking out an issue of an other user, I had some findings. I was tinkering around how to communicate them and maybe also try to suggest a solution in way that it is easy for you to check. I am not sure if it benefits you right now when we use templates for features and issues, to help you out concentrating on important things in life.
Maybe an contribution guide?

All the best to your family and you,
fi-do

@kensoh kensoh merged commit 3fae088 into tebelorg:master May 23, 2021
@kensoh kensoh changed the title [WIP]Feature/installation path Add support to define custom TagUI folder May 23, 2021
@kensoh kensoh changed the title Add support to define custom TagUI folder Add support to define custom TagUI installation folder May 23, 2021
@kensoh
Copy link
Member

kensoh commented May 23, 2021

Hi @fi-do and @Ashwani001, I've reviewed the PR and opted to do it by introducing a new function tagui_location().

This is added in v1.36, available with pip install rpa --upgrade

To use, set r.tagui_location('your_full_path') before calling r.init() or r.pack()

For example, following works on macOS and installs TagUI to a .tagui folder within Desktop folder.

import rpa as r
r.tagui_location('/Users/username/Desktop')
r.init()
r.url('https://google.com')
r.close()

If r.tagui_location('your_full_path') is not called to set a custom folder, the default will be used -

  • %APPDATA%/tagui folder for Windows
  • and ~/.tagui folder for Mac and Linux

With this function, users can also call r.tagui_location() to know where TagUI is installed by default.

I opted for this because the code change is minimal < 10 lines of code and has the lowest risk of breaking stuffs. @fi-do's code looks great and I understood all the parts. I thought to create a separate function to do this because having a parameter to init() and pack() which is only used by a minority of people can add clutter to majority of folks. With the new tagui_location() function, folks who wants to set a custom installation folder can easily do so.

For time being, I'm not adding this to readme. If more people wants to specify a custom folder then I'll add. Also, in v1.35 and issue #256, the problem with spaces on Windows (it can affect users with space in their usernames) is fixed. Users can also run the package from any folder now on Windows, including those with spaces. For macOS and Linux, no issue. So setting a custom folder becomes less critical to using the package.

@kensoh
Copy link
Member

kensoh commented May 23, 2021

Oh @fi-do for issues templates, I feel that it adds extra trouble for most users raising issues while only benefitting me for a minority of issues. Most issues we can't have all the fields filled up and having all fields filled is not relevant. Replication detail is the most important, but if a user forgets or don't know to provide that, then I'll ask if that is relevant for solving the issue.

For contribution guide, also no fixed guideline. Raising an issue to discuss before developing the code can help to prevent wasted time from user, but if it is already something a user has created for own use then sending a PR right away is ok. For this PR, if you didn't create a PR to add this feature, I probably won't end up adding tagui_location().

I made my implementation directly in your PR so that your contribution can be recorded in the project. Thanks!

@kensoh
Copy link
Member

kensoh commented Jul 11, 2021

Added tagui_location() and download_location() (for setting file download location from web browser) -

https://github.com/tebelorg/RPA-Python#api-reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants