maui icon indicating copy to clipboard operation
maui copied to clipboard

[Windows] Implement SearchBar CancelButtonColor on Windows

Open jsuarezruiz opened this issue 2 years ago • 0 comments

Description of Change

Implement SearchBar CancelButtonColor on Windows. Checking new issues and adding label, I've seen that 13507 happens basically because it's not implemented. This PR implement the changes in the CancelButtonColor proprerty. image

To validate the changes cacn use the SearchBar sample from the .NET MAUI Gallery or the added tests. image

Issues Fixed

Fixes #13507

jsuarezruiz avatar Mar 01 '23 15:03 jsuarezruiz

Hey,

sounds like a duplicate of #965 which is already resolved by #6737. This will be included in the 1.4 release.

fin swimmer

finswimmer avatar Feb 17 '23 13:02 finswimmer

I'm not sure if I'm interpreting the docs/patch correctly, but that only appears to be fixing the issue for installed scripts. Does Poetry install scripts in editable mode? When is a script installed vs not?

sys.argv[0] should be the absolute path of the module being imported. Python itself does this with python hello.py and python -m hello, and other tools expect this.

davidism avatar Feb 17 '23 15:02 davidism

If I remember correctly:

If you don't run poetry install or run poetry install --no-root, the root project as well as its scripts are not installed. Then, sys.argv[0] will still only contain the name of the script and not the absolute path.

If you run poetry install, the root project is installed in editable mode and its scripts will be installed in the target environment. Then, sys.argv[0] will be the absolute path of the installed script.

radoering avatar Feb 17 '23 16:02 radoering

Yeah, so in that case the linked PR won't help, since while you're developing you'll have the project installed in editable mode. Poetry currently turns poetry run dev into

/home/david/.cache/pypoetry/virtualenvs/hello-y1FJtfO2-py3.10/bin/python -c "import sys; from importlib import import_module; sys.argv = ['dev']; sys.exit(import_module('hello').run())"

import sys
from importlib import import_module
sys.argv = ['dev']
sys.exit(import_module('hello').run())

but it needs to generate a slightly different command that will set sys.argv to the imported module's filename, something like

import sys
from importlib import import_module
mod = import_module('hello')
sys.argv[0] = mod.__file__
sys.exit(mod.run())

davidism avatar Feb 17 '23 16:02 davidism

while you're developing you'll have the project installed in editable mode

I'm saying that with the PR sys.argv[0] will be absolute if the project is installed (even in editable mode). As far as I know, poetry does always install the root project in editable mode.

radoering avatar Feb 17 '23 17:02 radoering

even in editable mode

Ah ok, that makes more sense.

davidism avatar Feb 17 '23 17:02 davidism

There are +/- 2 sides here, I think:

  1. Poetry should be consistent in the behaviors of poetry run <script> and poetry shell, <script>. With #6737, this was resolved.
  2. sys.argv must be always self-callable (i.e. follow documented behavior), which in that case Poetry only guarantee with an installed script.

As a side note, I think we can achieve both with Poetry by not rewriting sys.argv on uninstalled scripts.

wagnerluis1982 avatar Feb 18 '23 13:02 wagnerluis1982

@davidism by checking documented behavior, I don't understand how you intend to rely on it.

After a few random checks, I got that python -c don't get what you wish.

For instance, I expected the following to print ['-c', 'import sys; print(sys.argv)'] in order to be self-callable, but didn't.

$ python3.10 -c "import sys; print(sys.argv)"
['-c']

wagnerluis1982 avatar Feb 18 '23 23:02 wagnerluis1982

This issue was noticed because of Werkzeug's reloader, but it's not actually related to fixing that issue or being able to relaunch a process. The issue with Poetry is that it's changing sys.argv[0] to something unexpected.

The sys.argv[0] = "-c" matches Python's documented behavior for the python -c code invocation, so that's also an acceptable fix, although it wouldn't provide launch details.

Fixing Poetry by making it use sys.argv[0] = "full path to module" would be best. That matches the behavior of python -m module, which Poetry is basically emulating with its entry point.


You proposed one solution, which is to not rewrite sys.argv. I think that's fine. You could also fix it by always rewriting it to imported_module.__file__ as I showed above, by only making a small change to the generated command.

davidism avatar Feb 19 '23 15:02 davidism

This issue was noticed because of Werkzeug's reloader, but it's not actually related to fixing that issue or being able to relaunch a process. The issue with Poetry is that it's changing sys.argv[0] to something unexpected.

Here you're overstating. Remember poetry run can run any command, not only python scripts. I compare to docker run command, if you ask to run, it will run. So, if I ask to run a script on the environment PATH (which is the case of installed script), I expect to behave like that, which poetry does well with the fix made on #6737. If you have your poetry on latest master, you're going to have this behavior.

The sys.argv[0] = "-c" matches Python's documented behavior for the python -c code invocation, so that's also an acceptable fix, although it wouldn't provide launch details.

It is never an acceptable fix, for the reason I mentioned above.

Fixing Poetry by making it use sys.argv[0] = "full path to module" would be best. That matches the behavior of python -m module, which Poetry is basically emulating with its entry point.

python -m relies on if __name__ == "__main__" in the module, poetry must not follow this. If you want it, you can simply go with poetry run python -m module.

wagnerluis1982 avatar Feb 19 '23 20:02 wagnerluis1982

The discussion here took a different path towards a different type of issue (if there is an issue at all). Since the issue originally reported by @delucca was fixed on #6737, I suggest the maintainers to close this as duplicate.

wagnerluis1982 avatar Feb 19 '23 20:02 wagnerluis1982

@davidism I recommend you to create a discussion (Discord or GitHub) with your point if you still feel that you need (or to create a new issue).

wagnerluis1982 avatar Feb 19 '23 20:02 wagnerluis1982

@wagnerluis1982 I think @davidism was able to create a workaround to handle it on Flask's side, but I honestly don't think this is the proper fix for that

I understand that poetry run is supposed to allow us handling anything (not only Python) that is currently in PATH, but this is counterproductive as soon as to do this we need to change Python execution in a way that damages the usability of Python scripts

For example, I have another PR that I think can be related to the same issue. On that issue, if we run a Python application without using Poetry we can successfully attach a debugger to it (using nvim-dap), but if we use Poetry it simply breaks as soon as the server launches

I'm still not 100% sure it is a related issue (I still need to debug it), but based on the current discussion I'm almost sure it is

delucca avatar Feb 20 '23 23:02 delucca

@delucca

I understand that poetry run is supposed to allow us handling anything (not only Python) that is currently in PATH, but this is counterproductive as soon as to do this we need to change Python execution in a way that damages the usability of Python scripts

Are we still talking about Poetry 1.3.2 behavior? If so, a fix is already provided, only waiting for 1.4.0 release to happen (very soon).

For example, I have another PR that I think can be related to the same issue. On that issue, if we run a Python application without using Poetry we can successfully attach a debugger to it (using nvim-dap), but if we use Poetry it simply breaks as soon as the server launches

Here, while may be related to the design decisions of poetry run, we are talking of a different issue and should be part of a different issue.


For all the things, do you mind to try to test with a Poetry installed on latest master?

wagnerluis1982 avatar Feb 21 '23 21:02 wagnerluis1982

Hi @delucca, poetry 1.4.0 is released. Can you give a check with your use cases?

wagnerluis1982 avatar Feb 28 '23 16:02 wagnerluis1982

@wagnerluis1982 just tried here and the issue remains 😢

delucca avatar Mar 02 '23 13:03 delucca

@davidism I also tried updating Flask to the latest version, but the issue persists

delucca avatar Mar 02 '23 13:03 delucca

That's because there's no new version of Werkzeug yet. But that shouldn't matter, if Poetry did add the full path to the Python file in sys.argv then it would start working.

davidism avatar Mar 02 '23 14:03 davidism

@davidism but you have the full path to the entry point (once is installed).

If you want the full path to the module, you can simply do poetry run python -m module

wagnerluis1982 avatar Mar 02 '23 16:03 wagnerluis1982

@wagnerluis1982 just tried here and the issue remains 😢

You mean the issue described in the issue?

If so, did you install the entry point via poetry install?

wagnerluis1982 avatar Mar 02 '23 16:03 wagnerluis1982

argv[0] is the script name (it is operating system dependent whether this is a full pathname or not)

I could see a case for argv[0] being the link /some/virtualenv/bin/show, but /absolute/path/to/example.py doesn't seem right at all

if you first activate the environment and then just run show, that's what happens.

Edit: or anyway, that's what happens on my operating system. In view of the "operating system dependent" I'd suggest that it's unwise to rely on the full path being available anyway

dimbleby avatar Mar 02 '23 16:03 dimbleby

Looks like everything works correctly with Poetry 1.4.0.

from flask import Flask

app = Flask(__name__)

@app.get("/")
def index():
    return "Hello, Poetry!"

def dev_server():
    app.run(debug=True)
[tool.poetry.scripts]
dev_server = "example:dev_server"
$ poetry install

$ poetry run dev_server
 * Serving Flask app 'example'
 * Debug mode: on
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.
 * Running on http://127.0.0.1:5000
Press CTRL+C to quit
 * Restarting with stat
 * Debugger is active!

$ poetry run flask -A example run --debug

 * Serving Flask app 'example'
 * Debug mode: on
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.
 * Running on http://127.0.0.1:5000
Press CTRL+C to quit
 * Restarting with stat
 * Debugger is active!

Both commands work as expected and the reloader works correctly.

poetry install is required, otherwise sys.argv[0] is still ["dev_server"]. I still think that could be fixed by using sys.argv[0] = mod.__file__ in the command Poetry generates internally, but poetry install should be required anyway so it's up to you at this point.

davidism avatar Mar 02 '23 16:03 davidism

I still think that could be fixed by using sys.argv[0] = mod.__file__ in the command Poetry generates internally, but poetry install should be required anyway so it's up to you at this point.

As said multiple times, makes no sense poetry run entry_point has a different behavior of poetry shell; entry_point

wagnerluis1982 avatar Mar 02 '23 20:03 wagnerluis1982

When not installed, entry_point as a standalone script doesn't exist to be called within poetry shell. So right now, for uninstalled scripts, Poetry generates an outright incorrect sys.argv[0] by setting it to the name of the script. To be correct, it could set it to "-c", or to be less correct but more helpful it could set it to mod.__file__.

But as I said, this works for installed projects now, you're welcome to close it if you don't want to address uninstalled projects.

davidism avatar Mar 02 '23 20:03 davidism

Adding to the discussion, I feel the wrongness feeling is being fueled by the fact that poetry run may have different behaviors regarding entry point (installed / non-installed).

So, I feel that poetry should forbid at all to run non installed entry points. What do maintainers think about it?

wagnerluis1982 avatar Mar 02 '23 20:03 wagnerluis1982

At the moment, I don't see a use case why it should be necessary to run non installed entry points. However, before we forbid it we should deprecate it and see if people do complain.

radoering avatar Mar 03 '23 05:03 radoering

@wagnerluis1982 just tried here and the issue remains 😢

You mean the issue described in the issue?

If so, did you install the entry point via poetry install?

oh, do I need to run poetry install again after adding the script line?

I'm running a installed entry point (the entry point of my app). I'll try it again to see if it works and let you know.

delucca avatar Mar 03 '23 13:03 delucca

@wagnerluis1982 okay, I ran poetry install and now it works as expected 😄

Since I didn't add any new dependencies, I didn't know I had to run poetry install before testing.

So, I think it is fixed 😄

delucca avatar Mar 03 '23 16:03 delucca

I open a discussion at https://github.com/python-poetry/poetry/discussions/7599 to we use to decide what to do with poetry run with unistalled scripts.

wagnerluis1982 avatar Mar 03 '23 21:03 wagnerluis1982

@wagnerluis1982 perfect 😄

I think we can close this one

delucca avatar Mar 03 '23 23:03 delucca