Skip to content

OWFileBrowser widget#795

Draft
ngergihun wants to merge 11 commits intoQuasars:masterfrom
ngergihun:owquickfile
Draft

OWFileBrowser widget#795
ngergihun wants to merge 11 commits intoQuasars:masterfrom
ngergihun:owquickfile

Conversation

@ngergihun
Copy link
Contributor

We wanted to have a quick and easy way to browse through lots of measurements quickly. To achieve we implemented a new widget that has a treeview-based file selection enabling much easier file browsing and loading. We think this it is a useful addition that makes the life of an end user easier and also enables the use of this widget in standalone apps.

@borondics
Copy link
Member

Thanks for this! I think it will be nice.


SIZE_LIMIT = 1e9

settingsHandler = PerfectDomainContextHandler(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has no domain-connected settings and therefore should not have a context handler.

@markotoplak
Copy link
Collaborator

Is this supposed to save the last selected file? I think it should save it a a schema_only settings.

@ngergihun
Copy link
Contributor Author

No, but maybe the sheet could be saved. Is it with: sheet = Setting(None, schema_only=True) ?

@markotoplak
Copy link
Collaborator

The last loaded file path should also be saved. Ideally, a RecentPath object is used for that setting. To keep the widget the as similar as OWFile, a list of RecentPath's with just one element could be used.

RecentPath already includes the reader and sheet setting.

@ngergihun
Copy link
Contributor Author

Okay, I'll make the changes to use RecentPath

@markotoplak
Copy link
Collaborator

On my Linux with Qt 6.8.1 I get:

File "/home/marko/dev/orange-spectroscopy/orangecontrib/spectroscopy/widgets/owfilebrowser.py", line 8, in <module>
    from AnyQt.QtWidgets import (
ImportError: cannot import name 'QFileSystemModel' from 'AnyQt.QtWidgets' (/home/marko/venv312/lib/python3.12/site-packages/AnyQt/QtWidgets.py)

I added a commit that fixes the issue.

@markotoplak
Copy link
Collaborator

General comments:

  • File type chooser should go under the file pickler.
  • Opening on click is a strong operation. Choosing a file should not make anything time consuming run by default. I'd add a checkbox where this can be enabled / disabled. By default it should be double-click.
  • How does this work on Windows where there are also different drives (C:, D:)?
  • Pathname display on Linux is wrong (I guess also on Macs). For me, it shows double slashes "/ /" for the start. (I do like how well does it interact though)

@markotoplak
Copy link
Collaborator

Here I see many functions from OWFile copied and maybe slightly edited. This makes code review needlessly hard.

I therefore suggest this widget is redone as a OWFile subclass so that there is no need to copy the shared function. For now, at least. This will also bring in all recent_path functionality.

@ngergihun
Copy link
Contributor Author

@markotoplak, thx for the suggestions. I'll work on a new version and come back with that for review.

@stuart-cls
Copy link
Member

stuart-cls commented Mar 18, 2025

Very interesting! Am I supposed to be able to select multiple datasets?

The string filtering does not work for me (but I like the idea).

I do have to ask, at what point do we have too many dataset widgets? We already have 6 (File, CSV, Datasets, SQL, Multi-File, TileFile). Perhaps this would be a better fit for orange-spectroscopy-prototypes while we test out how the functionality works with real usage?

enables the use of this widget in standalone apps

What do you mean by this?

Edit: I forgot my TileFile widget, that makes 6!

@borondics
Copy link
Member

Selecting multiple datasets would be nice, but then it opens a bit of a can of worms because we would need to check for dataset sizes to be consistent...

I have the same idea about having too many File readers. Not sure how to resolve this well. :( Any ideas would be great.

@ngergihun
Copy link
Contributor Author

ngergihun commented Mar 19, 2025

Hi @markotoplak ! I started the rewrite of the widget by subclassing OWFile. I try to reduce the code as much as possible, but I still need to reimplement some functions a bit differently due to the different components and interactions with the widget.

Also, when subclassing I get a warning, saying:
RuntimeWarning: subclassing of widget classes is deprecated and will be disabled in the future.
Extract code from OWFile or explicitly open it by adding openclass=True to class definition.
class OWFileBrowser(OWFile):

Also, OWFile has UI elements that we don't have and most of its functions refer to them, I have to create 'dummy' UI elements with the same name that's makes a bit of a mess.

If it is okay with the mentioned limitations, then I can continue.

@stuart-cls
Copy link
Member

I had the same problem with TileFile. I think in that widget I just ended up putting in a code-copy which is not great.

Really we need this: biolab/orange3#5376

@ngergihun
Copy link
Contributor Author

@stuart-cls, yes, I came across your OWTileFile when I was looking for inspiration and suspected that you copy-modified some of the code for this reason.

@borondics
Copy link
Member

Guys, what should we do with this widget? I mean we need to release it in the coming weeks but the question is what is the best way for now?

Subclass or not?

@markotoplak and @stuart-cls could you please comment (or help) how to fix the outstanding issues?

Thanks in advance!

@markotoplak
Copy link
Collaborator

Please make it a subclass for now. For ease of the review, mainly. Even though Orange is warning against it.

@ngergihun ngergihun marked this pull request as draft May 15, 2025 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants