Bug 492733 - ["Smart Importer"] Open Projects wizard scanning not cancelable
Summary: ["Smart Importer"] Open Projects wizard scanning not cancelable
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: IDE (show other bugs)
Version: 4.6   Edit
Hardware: All All
: P3 critical (vote)
Target Milestone: 4.6 RC1   Edit
Assignee: Mickael Istria CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 472614
Blocks:
  Show dependency tree
 
Reported: 2016-04-29 11:51 EDT by Markus Keller CLA
Modified: 2016-05-31 07:52 EDT (History)
3 users (show)

See Also:
eclipse.sprigogin: review+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2016-04-29 11:51:11 EDT
- File > Open Projects...
- start typing a path to a directory that contains a myriad of files, including the ZIP file you want to import

=> Workbench is blocked for ages, no way to cancel, no progress reporting

Expected: First think about whether it's a bright idea to scan folders on keystroke. You can decide not to follow the established UI practice (only process a path field when the user clicks a "Reload" button, or at least only when the field loses focus), but then you have to implement it in a way that works.

Anyway, the scanning needs to be cancelable.
Comment 1 Mickael Istria CLA 2016-05-02 08:20:31 EDT
The PR suggested for https://git.eclipse.org/r/#/c/70954/ should be a good fix for this issue.
Comment 2 Markus Keller CLA 2016-05-03 11:27:47 EDT
(In reply to Mickael Istria from comment #1)
> The PR suggested for https://git.eclipse.org/r/#/c/70954/ should be a good
> fix for this issue.

That fix may address follow-up issues, but not the root problems of this bug:
a) no progress bar and cancel button visible, Esc key doesn't cancel
b) scanning steals focus from "import source" field and blocks user from typing
Comment 3 Mickael Istria CLA 2016-05-03 11:45:31 EDT
(In reply to Markus Keller from comment #2)
> a) no progress bar and cancel button visible, Esc key doesn't cancel

I believe it's bug 491438

> b) scanning steals focus from "import source" field and blocks user from
> typing

Yes, I agree it would be nice to keep most of the page enabled and only "lock" the proposal part on scanning. And if user changes the source, Cancel the ongoing scanning and start a new one.
Comment 4 Markus Keller CLA 2016-05-04 08:43:10 EDT
No, bug 491438 is a separate problem.

This bug is completely deterministic. Your problem is that you completely neglect the protocol of IProgressMonitor:
- you don't call IProgressMonitor#beginTask(..)
- you pass the same monitor multiple times when calling ProjectConfigurator#findConfigurableLocations(File, IProgressMonitor). Use SubMonitors. If you don't understand the APIs or if they don't work, then ask Stefan Xenos.

Note that the convention is to pass uninitialized progress monitors to other methods, so the correct fix may or may not be in your code only.

Since progress reporting has obviously never been tested, you also need to check if the actual advancement of the progress bar works fine.
Comment 5 Mickael Istria CLA 2016-05-04 09:22:42 EDT
(In reply to Markus Keller from comment #4)
> This bug is completely deterministic. Your problem is that you completely
> neglect the protocol of IProgressMonitor:
> - you don't call IProgressMonitor#beginTask(..)
> - you pass the same monitor multiple times when calling
> ProjectConfigurator#findConfigurableLocations(File, IProgressMonitor). Use
> SubMonitors. If you don't understand the APIs or if they don't work, then
> ask Stefan Xenos.
> 
> Note that the convention is to pass uninitialized progress monitors to other
> methods, so the correct fix may or may not be in your code only.

Ok, I'll work on improving the progress reporting, following your tips and Stefan's documentation https://eclipse.org/articles/Article-Progress-Monitors/article.html
Comment 6 Eclipse Genie CLA 2016-05-04 10:31:10 EDT
New Gerrit change created: https://git.eclipse.org/r/72040
Comment 8 Brian de Alwis CLA 2016-05-20 10:50:16 EDT
Verified in 4.6.0.I20160519-1730
Comment 9 Lars Vogel CLA 2016-05-31 07:52:23 EDT
(In reply to Markus Keller from comment #0)
> Expected: First think about whether it's a bright idea to scan folders on
> keystroke. You can decide not to follow the established UI practice (only
> process a path field when the user clicks a "Reload" button, or at least
> only when the field loses focus), but then you have to implement it in a way
> that works.

Opened Bug 495007 for this.