Community
Participate
Working Groups
* edit the properties of a task repository * click validate * click finish * the repository is unnecessarily validated again This is pretty annoying as it gets me every time. If I manually validate, then there is no need to do it again when I click finish unless I have changed something. I think the Validate on Finish checkbox should be greyed out after a successful validation, and treated as unchecked, until the user makes a change to something other than the label.
Review https://git.eclipse.org/r/6013 created! Steffen, can you please review.
Created attachment 215763 [details] mylyn/context/zip
Thanks Frank!
I'm -1 on this change. The updated workflow should no longer require manual validation. Users are now expected to change settings and hit Finish. The proposed automation for toggling the validate on finish button won't be obvious and I am not aware of other dialogs in Eclipse that work that way. I'm inclined to mark this bug as wontfix.
I see the concern, but this is inconsistent with all other connectors. I think that this should be pushed from Bugzilla up to the framework, so that all connectors get this support. However, for existing users who are used to clicking validate, this is very annoying. Also, I know of at least one connector where validation is done during set up to retrieve the list of project areas from the server. For that connector there is no reason to validate again on finish, so there should be API to allow the box to be unchecked.
This is in the framework and all connectors should be leveraging the new API. If the connector did already validate and no settings were changed it could simple return a valid status without contacting the server again.
Thanks, that's exactly what should happen! The Bugzilla connector doesn't do that though. Because connectors have to opt in to get this functionality, most (that I know of) do not, and so users may not know what to expect.
Frank, I wonder if we could change your patch to generically implement that behavior: If a repository has been validated and nothing has changed the framework should shortcut the validation, e.g. by setting a flag in the page that the repository has already been validated in which case the validation method would return by default.
Another reason to do this is that sometimes validation takes a long time and is not cancellable.
(In reply to comment #8) > Frank, I wonder if we could change your patch to generically implement that > behavior: If a repository has been validated and nothing has changed the > framework should shortcut the validation, e.g. by setting a flag in the page > that the repository has already been validated in which case the validation > method would return by default. Yes we can do this. I think I have only to remove some lines of code for enable/disable the validateOnFinist button. A new review is coming soon.
new review created! Should I change the following SettingPage? * MockRepositorySettingsPage * OslcRepositorySettingsPage * TracRepositorySettingsPage * MyTracRepositorySettingsPage * WebRepositorySettingsPage
Let's do a review first. I would prefer if we implemented in a way that didn't require changes to the client implementations or at least generalized it but I haven't looked at the proposal, yet.
(In reply to comment #12) > Let's do a review first. I would prefer if we implemented in a way that didn't > require changes to the client implementations or at least generalized it but I > haven't looked at the proposal, yet. We need to add an ModifyListener or an SelectionListener to every Control that is relevant for validation od do you know an other way?
Could you intercept changes to the TaskRepository? Maybe compare the properties before and after apply is called?
(In reply to comment #14) > Could you intercept changes to the TaskRepository? Maybe compare the properties > before and after apply is called? Yes, please look in PatchSet 3. Only the following two properties label and Disconnected are ignored
Created attachment 215904 [details] mylyn/context/zip
That looks like it would work, assuming that the repository page doesn't do something weird and store some information somewhere else, which seems like a reasonable assumption. I"m not sure, though, why you implemented compareTo rather than just using equals(), as it doesn't seem like the order matters. Note that String.compareTo(null) will throw an NPE - I'm not sure whether you need to worry about that here.
Thanks Frank. It's too late to take this change on for 3.8 but we should consider it for the next cycle.
Frank, I have reverted the changes that were pushed to the master of o.e.m.commons. The compareTo() implementation does not look right and it's not clear to me why it's needed. These type of changes need to be accompanied by tests and should go through a code review before being pushed to master.
Steffen, I create patch set 4 in review 6013. Now we no longer need the change in o.e.m.commons. We use equals() instead of compareTo().
Created attachment 220311 [details] mylyn/context/zip
Steffen, can you please review this? Or should I look for an other reviewer?
I no longer have a "validate on finish" checkbox when running on master. So, if I can't validate (e.g. if I am offline), I can't finish. Was this change intentional?
(In reply to comment #23) > I no longer have a "validate on finish" checkbox when running on master. So, if > I can't validate (e.g. if I am offline), I can't finish. Was this change > intentional? No that is an regression from bug#253142. I put the fix soon.
Great, thanks Frank.
Not sure if I get the UX problem right here, but wouldn't disabling the "Validate" button if the "Validate on finish" is checked spare us the extra validation? Or do we want them both to be enabled all the time?
(In reply to comment #24) > No that is an regression from bug#253142. I put the fix soon. Frank, you'll need to rebase your patch on top of that change, as the checkbox is still not there.
(In reply to comment #26) > Not sure if I get the UX problem right here, but wouldn't disabling the > "Validate" button if the "Validate on finish" is checked spare us the extra > validation? Or do we want them both to be enabled all the time? Yes, sometimes you want to validate without finishing.
(In reply to comment #28) > (In reply to comment #26) > > Not sure if I get the UX problem right here, but wouldn't disabling the > > "Validate" button if the "Validate on finish" is checked spare us the extra > > validation? Or do we want them both to be enabled all the time? > > Yes, sometimes you want to validate without finishing. Do we actually still need this? If the finish button achieves the same result why have 2 buttons?
Are you suggesting to remove the Validate button? I often want to change the connection setting and see if it validates before I make other changes.
I often use the Validate button direct after filling in the password so I check the other properties after I know that the connection setting are valid.
pushed patch set 8 of review https://git.eclipse.org/r/#/c/6013/ so maybe we can do this for 3.10.
new patch set (9) that use guava AbstractRepositorySettingPage.equalsProperties
Created attachment 236471 [details] mylyn/context/zip
Looks like this is almost ready to be merged. Since we are past the cut off for 3.10 I have updated the milestone to the next release.
review is now in MASTER.
