Bugzilla – Full Text Bug Listing
| Summary: | When cloning a repository, if a URL is in the clipboard, use that | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Technology] EGit | Reporter: | Alex Blewitt <alex.blewitt> | ||||
| Component: | Core | Assignee: | Project Inbox <egit.core-inbox> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | enhancement | ||||||
| Priority: | P3 | CC: | eitan.koren, mathias.kinzler, matthias.sohn, mn | ||||
| Version: | 0.8.0 | ||||||
| Target Milestone: | 0.8.0 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Alex Blewitt
Pushed change as http://egit.eclipse.org/r/#change,805 with a dependency on http://egit.eclipse.org/r/#change,803 (Do I need to attach patch here as well?) If you push patch to gerrit you don't need to attach it in Bugzilla. Hence you have to accept the Eclipse contribution terms when registering with our gerrit instance. merged as 2854822da4c717e8df745486b440000639f80d49 Oops - forgot to dispose "clippy" in the acquisition. Let me submit a revision. merged fix as b0338c95ebd22bb87f3a592858cb96e50ff6d7b1 I found two problems with this: 1. filling the URI field from the clipboard automatically is confusing to the end-user: they won't understand that the value is coming from the clipboard, instead they would (at least I would) think that the value was stored somewhere in the preferences as "last used URI" or such. 2. the URI is always filled into the text field, no matter whether it points to a Git Repository or not. In order to fix 1. I believe the automatic filling of the field should be dropped. Instead, the "Paste" action should be used by the end-user. Since this is already available through standard SWT (through right-click or keyboard), we shouldn't need own coding for this. In order to fix 2. I guess we need better validation of the page. In the checkPage() method, currently if the scheme is "file", only file existence is checked. However, I think FileKey.isGitRepository(File) or something like this should be used to verfiy that the (file-) URI points to a Git Repository and the page should behave similar to when an invalid URI with scheme "git" is used (saying that it can't get remote repository refs). (In reply to comment #5) > I found two problems with this: > > 1. filling the URI field from the clipboard automatically is confusing to the > end-user: they won't understand that the value is coming from the clipboard, > instead they would (at least I would) think that the value was stored somewhere > in the preferences as "last used URI" or such. This behaviour is actively used in a number of other programs. It's not confusing, especially if the user has just gone to a remote page to copy the repository in in preparation for filling it in. > 2. the URI is always filled into the text field, no matter whether it points to > a Git Repository or not. That's because there's no way of knowing whether it's a valid Git repository or not, until you make the connection. The chances of this being a non-Git repository that just happens to be in the clipboard are fairly small in comparison with the chances of it being the git repository that you want to clone. > In order to fix 1. I believe the automatic filling of the field should be > dropped. Instead, the "Paste" action should be used by the end-user. Since this > is already available through standard SWT (through right-click or keyboard), we > shouldn't need own coding for this. That's a non-sequitur. You're asserting that we don't need or want this behaviour because a workaround is available. The point of this action was to make ti more seamless for putting the information in. > In order to fix 2. I guess we need better validation of the page. In the > checkPage() method, currently if the scheme is "file", only file existence is > checked. However, I think FileKey.isGitRepository(File) or something like this > should be used to verfiy that the (file-) URI points to a Git Repository and > the page should behave similar to when an invalid URI with scheme "git" is used > (saying that it can't get remote repository refs). It will do that when you click on 'finish' if it's not a valid Git URL. We shouldn't do this automatically. There's already a bug in the 'push' dialog whereby it tries to validate whether it's a valid Git repository by making repeated connections on a keypress-by-keypress basis, which is a DoS type bug. If the URL is a git: scheme, it's fairly obvious it's a Git URL. If it's a single http(s): or file: protocol, then it could well be a non-Git repository. However, the chances of it being a non-Git repository when you go into the Git clone page are pretty small - and if it turns out to be incorrect, changing it to the right/desired value is seamless. See bug 315575 as referenced in comment 6. The problem is that any kind of automated 'is it valid' action is going to cause further bugs like this - instead, you should check when the user clicks 'next' or 'finish' as appropriate (and take them back to the dialog if it's incorrect). After all, a web browser doesn't try to make an HTTP connection for each character typed ... it's when they hit 'enter' that the URL is validated to know if it's pointing to a resource or not. (In reply to comment #6) > (In reply to comment #5) > > I found two problems with this: > > > > 1. filling the URI field from the clipboard automatically is confusing to the > > end-user: they won't understand that the value is coming from the clipboard, > > instead they would (at least I would) think that the value was stored somewhere > > in the preferences as "last used URI" or such. > > This behaviour is actively used in a number of other programs. It's not > confusing, especially if the user has just gone to a remote page to copy the > repository in in preparation for filling it in. > Can you give an example or two, please? (In reply to comment #7) > See bug 315575 as referenced in comment 6. The problem is that any kind of > automated 'is it valid' action is going to cause further bugs like this - > instead, you should check when the user clicks 'next' or 'finish' as > appropriate (and take them back to the dialog if it's incorrect). After all, a > web browser doesn't try to make an HTTP connection for each character typed ... > it's when they hit 'enter' that the URL is validated to know if it's pointing > to a resource or not. Well, we are talking about a wiazrd and not a web browser here. In a wizard, we should only enable the "Next" button if the input on the current wizard page is "valid". Of course there is some latitude when it comes to define what "valid" means. But as far as I know, usability guidelines require that when the user hits "Next", the "next" page is to be displayed. We could then of course show an error message on that page, but the user would have to go back actively to the last page using "Back" in order to correct any input, then hit "Next" again to validate it again... But we should continue this discussion on bug 315575. (In reply to comment #9) > (In reply to comment #7) > > See bug 315575 as referenced in comment 6. The problem is that any kind of > > automated 'is it valid' action is going to cause further bugs like this - > > instead, you should check when the user clicks 'next' or 'finish' as > > appropriate (and take them back to the dialog if it's incorrect). After all, a > > web browser doesn't try to make an HTTP connection for each character typed ... > > it's when they hit 'enter' that the URL is validated to know if it's pointing > > to a resource or not. > > Well, we are talking about a wiazrd and not a web browser here. In a wizard, we > should only enable the "Next" button if the input on the current wizard page is > "valid". Of course there is some latitude when it comes to define what "valid" > means. But as far as I know, usability guidelines require that when the user > hits "Next", the "next" page is to be displayed. We could then of course show > an error message on that page, but the user would have to go back actively to > the last page using "Back" in order to correct any input, then hit "Next" again > to validate it again... > > But we should continue this discussion on bug 315575. It is not necessary for the information to be pointing to a repository at that stage - and even if the "next" button performed the check, you can still override it. In fact, if you look at the "CVS" page, it doesn't make any connection until you browse for modules and/or put in a name and click "finish". We should absolutely not be doing a one-login-per-keystroke. If the password is invalid or mistyped, it could easily trigger a server-side disabling of the account. My point is that this fix works as described - it takes what looks like a Git-known URL and pre-fills the values. Whether it points to a real Git URL should only be known at Finish time. Validation is are require fields filled in and syntactically correct. I vote for this bug to be closed as fixed, since the behaviour has already been signed off and approved by others as well as pushed to master. Created attachment 171675 [details]
wrong clipboard guess for clone wizard
At first sight I found this feature useful (hence I also accepted it) but in the meantime after some days of use my opinion changed: I used this now for a few days. Yesterday I had some really ugly experiences with this feature when finishing preparation for my demo camp session in Jena. I quite frequently hit nonsense matches in the clone wizard which forced me to manually delete the guess. Also when running the swtbot tests while working on http://egit.eclipse.org/r/#change,735 I observed that often some random text which happened to end up in the clipboard was picked by the clone wizard's url field. E.g. see the attached screenshot. Hence in the meantime I think we either need a less intrusive way to guess git URLs from the clipboard or we should revert this altogether. Maybe instead of implicitly pasting it to the URL field we could add it to the list of proposed URLs which can be reached by CTRL + space. But even then I think we need more reliable URL detection. I am also not sure if this isn't surprising users since it doesn't follow the standard clipboard usage pattern which requires an explicit paste action. Maybe Eitan as an experienced usability expert could comment on this. (In reply to comment #12) > At first sight I found this feature useful (hence I also accepted it) but in > the meantime after some days of use my opinion changed: We need to reduce the false positive rate to make this more useful > Hence in the meantime I think we either need a less intrusive way to guess git > URLs from the clipboard or we should revert this altogether. Maybe instead of > implicitly pasting it to the URL field we could add it to the list of proposed > URLs which can be reached by CTRL + space. But even then I think we need more > reliable URL detection. I suspect if we end up having other means (like Ctrl+Space) there isn't much difference from the user having to paste this manually. > I am also not sure if this isn't surprising users since it doesn't follow the > standard clipboard usage pattern which requires an explicit paste action. As I said, in some Mac apps that do this already, it's a pleasant surprise when it works. But it requires the detection to be smarter than it is. There's several things we can do to make it less intrusive before we give this up as a failed experiment: * The URL shouldn't be 'anything beginning with', it should terminate at a whitespace character or other non-URL valid char. So the example above shouldn't have had the <space> ref/... afterwards, which obviously breaks it. A regex may help here. * Grabbing HTTP(s) links is likely to be problematic. For these, we can further restrict it to those with '.git' in the URL itself. Whilst this will prevent some valid http URLs from being automatically pasted, it should avoid the problems where we end up with a bugzilla URL or similar ending up automatically in * The pasted in text should be automatically selected, so you can just overtype with a new value if it's a wrong one. I'll submit a patch for the above and we can see if it improves matters. With regards to the SWTBot tests - would it make sense to have this feature tested as well? It sounds like we would just need to do a Clipboard().setObject("") or something to avoid spurious test failures. Note that this pattern is used more on Macs (e.g. data detection and such like) so if Eitan has experience on the Mac it would be great. If not, then it might be the case of not having seen it before and so not considered appropriately. (In reply to comment #13) > We need to reduce the false positive rate to make this more useful yes this is crucial > As I said, in some Mac apps that do this already, it's a pleasant surprise when > it works. But it requires the detection to be smarter than it is. There's > several things we can do to make it less intrusive before we give this up as a > failed experiment: This matches my initial feeling when trying this out the first time. > * The URL shouldn't be 'anything beginning with', it should terminate at a > whitespace character or other non-URL valid char. So the example above > shouldn't have had the <space> ref/... afterwards, which obviously breaks it. A > regex may help here. > > * Grabbing HTTP(s) links is likely to be problematic. For these, we can further > restrict it to those with '.git' in the URL itself. Whilst this will prevent > some valid http URLs from being automatically pasted, it should avoid the > problems where we end up with a bugzilla URL or similar ending up automatically > in > > * The pasted in text should be automatically selected, so you can just overtype > with a new value if it's a wrong one. Yes this will help, if I can simply start typing to ignore the suggested URL it's not in my way. Maybe we could also make it visible that this is a value which has been suggested automatically by e.g. using a different font. How do the mentioned Mac apps handle that ? > I'll submit a patch for the above and we can see if it improves matters. With > regards to the SWTBot tests - would it make sense to have this feature tested > as well? It sounds like we would just need to do a Clipboard().setObject("") or > something to avoid spurious test failures. Yes I think we should have a test for the feature. I am not sure if it's good to delete the system global clipboard from an automated test, this could disturb developers. I also didn't see a test failure because of the clipboard content initializing the URL field probably because the UI tests are explicitly setting the URL field which overwrites the previously suggested value. > Note that this pattern is used more on Macs (e.g. data detection and such like) > so if Eitan has experience on the Mac it would be great. If not, then it might > be the case of not having seen it before and so not considered appropriately. Hopefully I get my ordered Mac soon, then I'll have a look. Which applications are using this approach ? pushed further improvements: - "Preselect URI preset from clipboard" http://egit.eclipse.org/r/#change,930 - "More strict rules for guessing clone URL" http://egit.eclipse.org/r/#change,928 I think with these improvements we can close this bug. Agreed. Marking as resolved. |