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

fixed ui.download not working in native mode #2884

Merged
merged 7 commits into from
Apr 23, 2024

Conversation

Smug246
Copy link
Contributor

@Smug246 Smug246 commented Apr 13, 2024

Feature request: #2877

@falkoschindler falkoschindler self-requested a review April 13, 2024 10:41
@falkoschindler falkoschindler added this to the 1.4.22 milestone Apr 13, 2024
@falkoschindler falkoschindler added the bug Something isn't working label Apr 13, 2024
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the pull request, @Smug246!

I just tested this simple test code on macOS:

ui.button('Logo', on_click=lambda: ui.download(b'Hello World', 'hello.txt'))

Unfortunately I get this error:

--- Logging error ---
Traceback (most recent call last):
  File "/Users/falko/.pyenv/versions/3.11.7/lib/python3.11/logging/__init__.py", line 1110, in emit
    msg = self.format(record)
          ^^^^^^^^^^^^^^^^^^^
  File "/Users/falko/.pyenv/versions/3.11.7/lib/python3.11/logging/__init__.py", line 953, in format
    return fmt.format(record)
           ^^^^^^^^^^^^^^^^^^
  File "/Users/falko/.pyenv/versions/3.11.7/lib/python3.11/logging/__init__.py", line 687, in format
    record.message = record.getMessage()
                     ^^^^^^^^^^^^^^^^^^^
  File "/Users/falko/.pyenv/versions/3.11.7/lib/python3.11/logging/__init__.py", line 377, in getMessage
    msg = msg % self.args
          ~~~~^~~~~~~~~~~
TypeError: not all arguments converted during string formatting
Call stack:
  File "/Users/falko/.pyenv/versions/3.11.7/lib/python3.11/site-packages/webview/platforms/cocoa.py", line 283, in download_completionHandler_error_
    logger.error('Download error:', error)
Message: 'Download error:'
Arguments: (Error Domain=NSURLErrorDomain Code=-1002 "unsupported URL" UserInfo={NSLocalizedDescription=unsupported URL, NSErrorFailingURLStringKey=blob:http://127.0.0.1:1234/2c9d12b1-7c39-4d96-8294-1b8a5c8d5db0, NSErrorFailingURLKey=blob:http://127.0.0.1:1234/2c9d12b1-7c39-4d96-8294-1b8a5c8d5db0, _NSURLErrorRelatedURLSessionTaskErrorKey=(
    "LocalDownloadTask <7847E5B6-3828-4AD4-8FEE-E19F85617668>.<1>"
), _NSURLErrorFailingURLSessionTaskErrorKey=LocalDownloadTask <7847E5B6-3828-4AD4-8FEE-E19F85617668>.<1>, NSUnderlyingError=0x12ca33bc0 {Error Domain=kCFErrorDomainCFNetwork Code=-1002 "(null)"}},)

Do you have an idea what's going on? Or do you have a different test to show that your fix is working?

@Smug246
Copy link
Contributor Author

Smug246 commented Apr 13, 2024

ah it may be to do with macos. im on windows 11 and dont have a mac to test. im also on python 3.12.2

@Smug246
Copy link
Contributor Author

Smug246 commented Apr 13, 2024

uncapped_edit_1713025834898.mp4

no problems for me with ur example

@Smug246
Copy link
Contributor Author

Smug246 commented Apr 13, 2024

Could you test this example and see if it works?

import webview

if __name__ == '__main__':
    webview.settings['ALLOW_DOWNLOADS'] = True
    window = webview.create_window('Simple browser', 'https://pywebview.flowrl.com/download')
    webview.start()

@falkoschindler
Copy link
Contributor

Interesting:

  • Your plain pywebview example works. And the "ALLOW_DOWNLOADS" flag matters.
  • A NiceGUI test with the same URL works, independent of the "ALLOW_DOWNLOADS" flag:
    ui.button('Logo', on_click=lambda: ui.download('https://pywebview.flowrl.com/download/test-data.bin'))

Anyway, enabling this flag seems to help in some cases. But I wonder if we really should enable it by default, since pywebview probably disabled it for a reason. And technically it would be a breaking change.

We could easily make it configurable though: Adding settings to NativeConfig

@dataclass(**KWONLY_SLOTS)
class NativeConfig:
    start_args: Dict[str, Any] = field(default_factory=dict)
    window_args: Dict[str, Any] = field(default_factory=dict)
    settings: Dict[str, Any] = field(default_factory=dict)
    main_window: Optional[WindowProxy] = None

and using them

    webview.settings.update(**core.app.native.settings)

before creating the window. We could even print a warning if the user calls ui.download in native mode without having this flag enabled.

@falkoschindler falkoschindler modified the milestones: 1.4.22, 1.4.23 Apr 15, 2024
@Smug246
Copy link
Contributor Author

Smug246 commented Apr 16, 2024

so although that error occurs on mac you still think it should be default setting. i feel like that would cause numerous issues.

@falkoschindler
Copy link
Contributor

Let me clarify: I think we should make webview.settings configurable with ALLOW_DOWNLOADS being false by default. This way we don't introduce breaking changes and give full control to the user.

@Smug246
Copy link
Contributor Author

Smug246 commented Apr 16, 2024

oh right i misunderstood. i'll take my best shot at it

@Smug246
Copy link
Contributor Author

Smug246 commented Apr 16, 2024

should i make it configurable through ui.run() parameters?

@falkoschindler
Copy link
Contributor

As mentioned above, I think we should add settings to NativeConfig. Then the user can configure it via

app.native.settings['ALLOW_DOWNLOADS'] = True

like it's already possible for app.native.window_args and app.native.start_args:
https://nicegui.io/documentation/section_configuration_deployment#native_mode

@Smug246
Copy link
Contributor Author

Smug246 commented Apr 22, 2024

what do you think?

@Smug246
Copy link
Contributor Author

Smug246 commented Apr 22, 2024

had to make a quick revision because i confused myself lol

@falkoschindler falkoschindler self-requested a review April 22, 2024 15:21
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks, @Smug246!
Looks good to me. 😎

@falkoschindler falkoschindler merged commit cb38b02 into zauberzeug:main Apr 23, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants