Bugzilla – Full Text Bug Listing
| Summary: | [preferences] Should be able to export from 'Formatter' preference page | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Dani Megert <daniel_megert> | ||||||||
| Component: | UI | Assignee: | Raksha Vasisht <raksha.vasisht> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||
| Severity: | enhancement | ||||||||||
| Priority: | P3 | CC: | martinae | ||||||||
| Version: | 3.0 | ||||||||||
| Target Milestone: | 3.6 M1 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Dani Megert
Agree that exporting/importing should consider the formatter as well. However having different profiles that can be managed separatly allows easy sharing of formatter settings. Nothing we can do here for 3.0 since core's preference story got postponed > 3.0 and introducing out own implementation doesn't make to much sense. The page should have an Export... button. See also bug 279415. The Export... button on the 'Formatter' preference page would export ALL current profiles while the Export... button in the Edit dialog only exports the current one. Same applies to 'Clean Up' page. Created attachment 140866 [details]
Patch with the fix.
Added a new Export All... button to the formatter page which exports all the current profiles to a new file. Also modified the Import... code to import multiple profiles at once.
First time try of the patch causes a NPE and hence which is really bad. Please always try/test all the (new) options/buttons before you attach a patch. Why do you use the term 'save' in the code for the export functionality? Either it is 'save' or it is 'export' but mixing it is not good. I quickly glanced over the existing 'Export' buttons and all of them have 'x' as their mnemonic. What is the reason for changing this in your button? Created attachment 141296 [details]
Improved patch
(In reply to comment #8) > First time try of the patch causes a NPE and hence which is really bad. Please > always try/test all the (new) options/buttons before you attach a patch. It was NPE coming from file creation. I fixed that. > Why do you use the term 'save' in the code for the export functionality? Either > it is 'save' or it is 'export' but mixing it is not good. I saw that all of the existing import related terms named 'load' and export related terms named 'save', in methods and NLS strings. So i named Export All as 'saveAll' in all cases to maintain consistency. > I quickly glanced over the existing 'Export' buttons and all of them have 'x' > as their mnemonic. What is the reason for changing this in your button? > Oh i overlooked that point. I have changed it back to 'x'. Thanks! > Either it is 'save' or it is 'export' but mixing it is not good. It's still mixed which is bad in this case. Maybe I wasn't clear enough about this. Often there are several patterns that could be followed and it's sort of a gut feeling which one to follow. In this case when adding a button that does 'X' then all corresponding code must be named that way. The mismatch in the current code is most likely for historical reasons where it was decided to relabel the button from "Save" to "Export" but wanted to keep the code changes minimal. You are reusing a string from the export profile functionality/dialog. This is not a good idea. Please use a new key for the overwrite message. In addition, it is more readable if you group all the strings and constants for the new functionality together instead of spreading them all over the file: CodingStyleConfigurationBlock_save_profile_* belongs to one functionality/dialog with its messages and CodingStyleConfigurationBlock_save_profiles_* is another. They should be kept in separate blocks. >Oh i overlooked that point. I have changed it back to 'x'. Thanks! OK, but my initial question was *why* you used 'e' in the first place ;-) Created attachment 142359 [details]
Patch with changes
Changed all "save" to "export" for Export All... and grouped them together.
Patch is good. I only removed the following code:
dialog.setFileName("formatterprofiles"); //$NON-NLS-1$
because that code is also used to export clean up profiles and formatterprofiles" is not good there. This could be improved by getting the initial name from the subclasses.
Fixed in HEAD.
Available in builds >= N20090731-2000.
>Verified for 3.6 M1 using I20090803-1800 on Linux-GTK.
|