Bug 291339 - validateEdit not called for launch configuration stored locally
Summary: validateEdit not called for launch configuration stored locally
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Debug (show other bugs)
Version: 3.4.2   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.5.2   Edit
Assignee: Michael Rennie CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2009-10-05 08:23 EDT by Krzysztof Daniel CLA
Modified: 2010-07-26 10:09 EDT (History)
4 users (show)

See Also:
Michael_Rennie: review+


Attachments
proposed solution (4.38 KB, patch)
2009-10-06 10:09 EDT, Wojciech Galanciak CLA
no flags Details | Diff
updated patch (4.50 KB, patch)
2009-10-13 13:05 EDT, Michael Rennie CLA
no flags Details | Diff
updated patch with run cancel (6.06 KB, patch)
2009-10-20 19:33 EDT, Wojciech Galanciak CLA
Michael_Rennie: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Daniel CLA 2009-10-05 08:23:49 EDT
Reproduction steps:
1. Create launch configuration and store it inside a project
2. Make the file read-only
3. Launch Run configurations
4. Observer error message
  "The file associated with this launch configuration is read-only and cannot be modified".

Comments:
 Editors consult the user about removing read-only flag.
 The issue is especially annoying if the read-only flag is automagically set up by the source control.
 
When selecting particular file which is stored in the workspace, validateEdit should be called.
Comment 1 Darin Wright CLA 2009-10-05 10:24:49 EDT
What sort of launch configurationis this? Launching a configuration does (should) not always modify it.
Comment 2 Michael Große CLA 2009-10-05 12:51:02 EDT
This happens for Java launch configuration.
Comment 3 Krzysztof Daniel CLA 2009-10-05 13:25:03 EDT
But it is very probable that launch configuration will be modified. It is a good reason to validateEdit.
Comment 4 Darin Wright CLA 2009-10-05 14:59:44 EDT
Currently, the launch dialog disables editing of a config when it is read-only (and displays an error message). The core infrastructure does call validateEdit(...) before attempting to write the file (however, that code is never called in this case, since the UI disables editing).

Instead, the launch dialog should behave similar to an editor, calling validateEdit after the first edit is made in the launch config. Perhaps the error message should change to a warning initially, and display the status of the validateEdit, if the operation fails. Core infrastructure should continue to call validate edit as always.
Comment 5 Wojciech Galanciak CLA 2009-10-06 10:09:53 EDT
Created attachment 148884 [details]
proposed solution

There is one other idea to solve this problem. 

The problem with calling validateEdit after first modification is harder than in normal editor because in launch configuration dialog we have more than one event handler (each field, button has its own listener). 

I think that the better way is to allow user to change properties and call validateEdit when he/she chooses apply option. If user chose yes, we make file writable and write all changes, otherwise we do nothing (all changes that he/she has made in properties still are visible). 

I've changed error message to a warning message which is visible until user won't apply changes. Warning message is the same as error message but it should be changed because user can misunderstand it.
Comment 6 Krzysztof Daniel CLA 2009-10-12 07:18:32 EDT
Darin,

Could you reveiwe Wojtek's patch?
Comment 7 Michael Rennie CLA 2009-10-13 13:01:55 EDT
(In reply to comment #6)

> Could you reveiwe Wojtek's patch?

testing the patch, it feels to me that we could make this even simpler: 

1. we have to keep the call to canSave() when computing the dirty state - which allows tabs to directly interact with the saveable state of the configuration.

2. since we are enabling the apply / revert buttons we could just not show a warning at all about it being read-only since you will be prompted with a resolution (unset the read-only flag or cancel the save) when you try to run / switch configurations / press apply.
Comment 8 Michael Rennie CLA 2009-10-13 13:05:16 EDT
Created attachment 149454 [details]
updated patch

This alternate patch enables the apply / revert buttons as the other patch does, but does not show a message about the configuration being read-only. It also restores the call the canSave().
Comment 9 Michael Rennie CLA 2009-10-13 13:08:47 EDT
Christopher, does the alternate patch work for you?
Comment 10 Darin Wright CLA 2009-10-13 15:29:07 EDT
If the user presses "Run/Debug", they are prompted, but when they click "No", the config launches without saving (i.e. the old config is launched). I wonder if this should cancel instead?
Comment 11 Wojciech Galanciak CLA 2009-10-20 19:33:37 EDT
Created attachment 150039 [details]
updated patch with run cancel

There is Michael's patch with handled situation when user presses "no" when tries to run unwritable configuration. In this patch we just back to launch configuration editor. Another opiton is to close this dialog when no was pressed. I still think we should warn user (by a warning message at the top of the dialog) that this launch conf is read-only because he probably doesn't want to spend a time for modifications and after that find out that it is unwritable and he doesn't want to modify it for some reasons. Of course it is hypothetical situation.
Comment 12 Darin Wright CLA 2009-10-22 09:38:52 EDT
+1. works for me. I'm happy without the warning. The validate-edit generally resolves the issue anyway.
Comment 13 Michael Rennie CLA 2009-10-22 09:40:11 EDT
+1 from me
Comment 14 Darin Wright CLA 2009-10-22 09:40:52 EDT
Fixed in HEAD.
Comment 15 Michael Rennie CLA 2009-10-22 10:13:39 EDT
verified
Comment 16 Darin Wright CLA 2009-10-28 09:41:38 EDT
Re-opening to fix in 3.5.2
Comment 17 Darin Wright CLA 2009-10-28 10:58:24 EDT
Released to 3.5.2. Missed today's build - will be in next week's maitenenace build.