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

Distance File: Don't accept dropped .xlsx files #6601

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Oct 20, 2023

Issue

When dropping an .xlsx file onto canvas, the user has to choose between Distance File and File, which is confusing (because these are widget names). The user almost always wants to load an .xlsx with File.

Description of changes

Distance File's functionality now no longer accepts .xlsx files.

Includes
  • Code changes
  • Tests

@janezd janezd force-pushed the distance-file-disable-drop-xlsx branch 2 times, most recently from 44ac0ef to 7b324f9 Compare October 20, 2023 09:03
Orange/widgets/unsupervised/owdistancefile.py Show resolved Hide resolved
@@ -158,7 +160,12 @@ def parametersFromFile(self, path):
return {"recent_paths": stored_recent_paths_prepend(self.WIDGET, r)}

def canDropFile(self, path: str) -> bool:
return os.path.splitext(path)[1].lower() in (".dst", ".xlsx")
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

You could keep the old condition (only .dst) for better performance.

@@ -158,7 +160,12 @@ def parametersFromFile(self, path):
return {"recent_paths": stored_recent_paths_prepend(self.WIDGET, r)}

def canDropFile(self, path: str) -> bool:
return os.path.splitext(path)[1].lower() in (".dst", ".xlsx")
try:
distances = DistMatrix.from_file(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

This may take too long for big files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What was the file dimension again? Can you tell me which file it was - or send it? I tried 600x1000 and takes a few seconds.

What I discovered was that, unfortunately, most time is spent in opening the Excel file. Hence, the File widget will take more or less the same time as well.

Of course, with this PR, the file is read twice and we must prevent this. I'm considering decorating functions that open excel files with a lru cache of size 1 that would remember the last read excel workbook - of course checking not only the file name but also the timestamp. In this way - and given that opening the file takes majority of time - the time spent in owdistancefile would be saved in owfile because it would reuse the workbook object.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was 1000x1000 .xlsx.

@janezd janezd force-pushed the distance-file-disable-drop-xlsx branch from 7b324f9 to 69a7d1b Compare October 29, 2023 13:40
@janezd
Copy link
Contributor Author

janezd commented Nov 13, 2023

Note: taking a file and dragging it around the canvas triggers an incessant, continuous stream of calls to canDropFile, which freezes the app (and possible also Finder or another part of (another) OS). This posed no problem when canDropFile wasn't sniffing the file, but just looked at its name. For this PR to work, this has to be fixed, too.

@janezd janezd assigned janezd and unassigned VesnaT Dec 8, 2023
@janezd
Copy link
Contributor Author

janezd commented Jan 15, 2024

@ales-erjavec, can we assume that dragMoveEvent is always preceded by dragEnterEvent?

Could we avoid incessant calling of canHandleDrop in orangecanvas.document.interations.DropAction.dragMoveEvent, by changing if self.canHandleDrop(event) to if self.__designatedAction is not None?

@ales-erjavec
Copy link
Contributor

canHandleDrop should not do anything computationally expensive. Only check filename, possibly read a limited amount of the file (e.g. magic header byte[s])...

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.

3 participants