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

Get drop information out from windowview #3378

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

EmberLightVFX
Copy link
Contributor

This is my WIP PR for issue #3372
Right now all it does is allow you to drag and drop files into the webview and it will print out the path to the file from the main thread.

The issue I'm still having is how to access the running nicegui instance on the main thread as currently the check for droped files happens in a separate thread that's running from the main thread (not multiprocessing).

Are there any other way for me to check the drop_queue from within the main thread and from there create an event that could be picked up like ui.on("drop", my_func)?

@EmberLightVFX
Copy link
Contributor Author

Got it working!!!
I had to create a new widget to fetch the file drop. The positive with this is that you have have multiple drop-locations.

2024-07-22_23-18-15.mp4

There are two problems still tho that I would love to get some help with.

  1. Closing the native window doesn't close the listening thread I'm running
  2. When auto-reload is enabled this doesn't work. I think it's because the extra subprocess that gets created and information goes to the wrong process.

I'll post the data tomorrow as it's quite late here 😄

@EmberLightVFX
Copy link
Contributor Author

Added all files to make the drag and drop working and included a _example.py in the root dir for you to try it out.
I have added a new element called drop_zone. It's just a wrapper like row or column and the user needs to fill the content of the drop zone.
When a file or folder is being draged over the zone the class dragover will get added so the user can change the style of the zone.

Drop zone does still not work with refresh=True as drop_queue.put(file.get('pywebviewFullPath')) (in the native_mode.py file) gets applied on the refresh-thread and not the thread the user is interacting on. I don't know how to really solve this...
But I got the closing working!

@falkoschindler
Copy link
Contributor

Thanks for the pull request, @EmberLightVFX!
Unfortunately, we haven't found the time to look into it yet. We're currently focusing on the release of version 2.0 and other pressing matters. So we hope you find a way to fix the remaining issues with the implementation. Maybe someone from the community can jump in and assist?

@falkoschindler falkoschindler added the help wanted Extra attention is needed label Aug 19, 2024
@EmberLightVFX
Copy link
Contributor Author

No worries! I know you're busy with other stuff :) I haven't had much time myself to look into the problems but when I get some time I'll do my best. Any community help is highly appreciated tho!

@EmberLightVFX
Copy link
Contributor Author

I finally got some time to try and tackle this again.
I think I have found a way to not create a separate thread to fetch when the drop-information from webview but currently I don't know how to emit/broadcast the data to any object listening through dropzone.on("drop", func) like I did before when the emit came from within dropzone.js.

So quick question. Are there any way to emit/broadcast directly from the python code?

@EmberLightVFX
Copy link
Contributor Author

@falkoschindler
Forgot to tag you in case you wouldn't see my message

@falkoschindler
Copy link
Contributor

Hi @EmberLightVFX, thanks for the update!

Are there any way to emit/broadcast directly from the python code?

I'm not quite sure if I understand the question.
Isn't window.dom.document.events.drop conceptually different to __file-dropped? One is emitted when dropping anywhere on the document, the other is emitted when dropping onto the drop zone. This means it is impossible to know which ui.drop_zone drop handlers to call from within on_drop in native_mode.py.

@EmberLightVFX
Copy link
Contributor Author

EmberLightVFX commented Feb 17, 2025

@falkoschindler
The difference between window.dom.document.events.drop and __file-dropped is that file-dropped is a javascript event while window.dom... is an event from pywebview and it includes the full path to what ever you just dropped. That's the information I'm trying to catch.

I just pushed an update where I added a asyncio check when the drop zone gets a __file-dropped event and it checks for data in the drop_queue (mp.Queue()).
At the same time window.dom.document.events.drop executes the on_drop function in native_mode.py in webview's thread. In that function we set the data for drop_queue that the drop zone now can read and thus we have passed the file/folder information have bin passed.

A bit cumbersome but it's the only way in how NiceGUI and webview works.

So, how the drop zone have the path information but need to send the emit to everything that's listening.
The current way I'm doing it is to now run self.run_method("drop_emitter", data) to pass the data to a javascript method and that method simply sends out the emit.
There would just be less failure points if I could send the emit directly from the python code instead of having to go through javascript.

@EmberLightVFX
Copy link
Contributor Author

Btw, I left some print functions around just to be able to see what executes and when. I'll remove these when everything works as expected 😄

@EmberLightVFX
Copy link
Contributor Author

I have went through the code and fixed everything so it's now working as expected!
The only problem I have left is that the drop zone doesn't work if reload is set to True. There is some thread-comunication problem that would need to be sorted out.
And if it's possible to emit the data directly from python instead of javascript to reduce failure points, but the drop zone do work in its current form.

@EmberLightVFX
Copy link
Contributor Author

So I have bin looking through the code and I don't see a way to comunicate to the correct thread as the reloadis done by uvicorn.
I'm quite stuck. Any idea on how to sort this out?

@EmberLightVFX EmberLightVFX marked this pull request as ready for review February 18, 2025 23:47
@EmberLightVFX
Copy link
Contributor Author

From my understanding there isn't really any good way to get drop zone and reload to work at the same time because of all the different threads going on.
So my suggestion would be to simply show a warning that drop zones won't work if reload is enabled when launching the gui.
Would that be an acceptable solution @falkoschindler ?

Except that I think this PR is ready for review. I have still left my example python file but when all is accepted I'll remove it.

@EmberLightVFX
Copy link
Contributor Author

EmberLightVFX commented Feb 22, 2025

Took some time to change the event usage from ui.drop_zone.on("drop_zone", func) to ui.drop_zone(on_drop=func) to match other NiceGUI controls, did some cleanup and added documentation :)

@falkoschindler
Copy link
Contributor

Hi @EmberLightVFX, I really appreciate the work you're putting into this PR! Hopefully we can turn it into a useful feature.
Right now I'm still pretty confused:

  1. The discussion is getting long and I'm struggling to keep up with the development. So I might miss a point here and there.
  2. The diff is very hard to read if you keep reformatting unrelated code. In 1850ca9 and 99302e9 I already reverted most of it to unveil actual changes, but you seem to have repeated those changes.
  3. I still wonder how you want to use the window-related "window.dom.document.events.drop" event to trigger a handler which is listening for a drop on an element. Isn't this conceptually ambiguous? But as I said above, I might be missing something important here.
  4. The script _example.py isn't working for me. I don't see any print statement or notification including a filepath.

@EmberLightVFX
Copy link
Contributor Author

Yeah sorry for all updates I wrote here. Maybe a big too much.
To answer your notes:

  1. Sorry about that. I'll try to fix it. This was caused by me using Ruff for the formatting instead of autopep8. I'm now using autopep8 so it should be ok for future PRs.

  2. I'll try to explain what happens when a user drops a file into a drop zone as good as I can.

    1. The user drop a file into a zone.
    2. The zone's javascript-part gets a drop event in the DOM and sends out the event __file_dropped (This event only gets what modifier key might have bin hold down when the file gets dropped and the files filename and extension, not the path)
    3. The zone's python-part listens for the __file_dropped event and when it gets it it starts to check for when drop_queue gets data attached to it.
      All this happens induvidually per drop zone.
    4. While step 2 and 3 happens Webview also gets a drop event when a file gets dropped into its window. This event contains the full path to the file that got dropped. When this happens drop_queue will get set with the path to the files that got dropped.
    5. The zone's python-part that is listning for changes to drop_queue now gets the file path from webview and will send out the info to all listening event handlers.
  3. That's weird.... What OS are you running the test in? I have to see if I can find a way to check what web engine webview is using. It might be something to do with that maybe?

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 @EmberLightVFX! And sorry for the confusion. I just tested _example.py once again and it seems to be working. Maybe I dropped onto the second drop zone which isn't connected to an event handler.

Now that I can see your changes more clearly, I started reviewing them in more detail. My two main concerns are the following:

  1. The event mechanism sounds rather complicated. I'll look into it more closely and maybe we can find a simplification somewhere. But requiring native=True and reload=False is quite a limitation. It would be great to fix it for reload=True. And ultimately I would hope to support ui.drop_zone for native=False as well.

  2. The amount of code for the "hover style" is quite significant, including additional events just to toggle the Tailwind classes. Would something like

    ui.drop_zone().classes('outline-gray hover:outline-dashed')

    be too much of a simplification? It switches even when hovering without dragging a file, but it simplifies the code sooo much.
    If we were to keep the extra styling parameter(s), I'd try to handle the drag events right in JavaScript and switch classes and/or style there without involving the server. This reduces the need for these two events as well as needless latency.
    Another remark regarding Tailwind classes: There is an option to run NiceGUI without Tailwind. For this scenario built-in elements shouldn't rely on Tailwind but use plain CSS instead.

@EmberLightVFX
Copy link
Contributor Author

I'll go through all your points when I get some time over but let me answer your two notes first:

  1. There might be some way to solve that reload can be true. It's a real wrangle to manage all the threads and processes that NiceGUI creates with native and reload 😅
    Sadly it's imposible to get the file path in the webbrowser as it's against Javascript security to get direct file paths.

  2. I totally agree. I'm currently re-writing that part of the code. Instead of having a hover_style and cleared_hover_style argument for drop_zone I'm gonna use .classes() and .hover_classes() for the user to set the styles.
    So ether the user can write it out like you did with hover:outline-dashed in the classes() function or they can use .hover_classes("outline-dashed") for the class to only change when you hover a file over the drop zone.

  3. I'll change the internal tailwind css with plain css :)

@EmberLightVFX
Copy link
Contributor Author

EmberLightVFX commented Feb 26, 2025

I have now updated the code with all your comments.

I have now also added two extra classes to the drop zone element.
hover_classes and hover_overlay_classes.
With these the user can pick to ether change/update the base class using hover_classes for example to change the background color on hover.
With hover_overlay_classes the user can access a div that lives above the drop zones content, so with it you can create borders or add a background color with opacity above everything (see _example.py for better understanding).

I think this is the best way to let users get the style they want for the drop zones.
Currently you have to add relative to the base class and absolute to the overlay class manually for the overlay to cover the drop zone div. Maybe it's best to just always attach these for ease of use for the users?

Things left to do but I'll wait with until we have decided how everything should be working:

  • Update docs
  • Change docs to use styles instead of tailwind
  • Add hover_styles and hover_overlay_styles
  • Remove _example.py

@falkoschindler
Copy link
Contributor

Ok, thanks for the update! I still have some questions though:

  1. Why do we need the new drag counter? It wasn't there before.
  2. Do you really think we need to catch queue.Empty even if we check for drop_queue.empty()? I'd love to further simplify this code.
  3. In _handle_file_drop you're checking core.app.config.reload. But this is pretty late. Shouldn't we warn or raise in the initializer? Or what else could we do if reload=True?
  4. Same for native=False: What should happen in this case? Currently the drop simply doesn't work. I know that we don't get file paths from the browser. But could we do something different? Or should we raise in the initializer?

Actually, when I think about it: We cannot raise when creating ui.drop_zone because ui.run might not have been called. So we would need to postpone the check to on_startup.

@EmberLightVFX
Copy link
Contributor Author

  1. Why do we need the new drag counter? It wasn't there before. It had something to do with how the drop-event was bubbling when having overlays on the drop zone but it seems like it works as expected now any way :S So no need for them!
  2. The catch isn't longer needed. It was part of the old code. So hard to fully know what all code is for after all changes 😅
  3. I like giving a warning when a file gets dropped so you get some responsse rather than nothing happens. While you develop you also might not want a notification that drop_zones won't work each time a reload happens. Thoughts?
  4. A notification like when reload=True can be added. Where can I check if native has bin set to true? It doesn't seem like the native bool is being saved anywhere.

)
for drop_handler in self._drop_handlers:
handle_event(drop_handler, args)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

@EmberLightVFX This return prevents the while loop from looping. Should we remove it, or replace while not drop_queue.empty(): with if not drop_queue.empty():?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants