cloudfoundry-attic / eclipse-integration-cloudfoundry

Cloud Foundry Integration for Eclipse
Apache License 2.0
41 stars 47 forks source link

Add operation buttons to Env Variables wizard page. #20

Closed Phanatic closed 10 years ago

Phanatic commented 10 years ago

Screenshot of what the new buttons/actions look like Screenshot I'm using the "Add Service" icon to represent adding new Environment variables. Questions : 1.Is this overloading the icon too much? 2.Should I be using a different icon? 3.I looked in the icons directory and couldn't find anything suitable.

Phanatic commented 10 years ago

Also, I kept the old context menu around, for backward compat. Please let me know if I should remove it.

nierajsingh commented 10 years ago

Hi,

Thanks very much for working on this and submitting a solution so quickly. For this particular page we would like to use a similar layout as other Eclipse wizards with add/edit/delete buttons in a separate vertical column on the right side, and it is what we had in mind as a solution. Please let us know if you would like to make the changes so that the environment variable wizard page more close resembles this screenshot:

eclipse_wizard_edit_buttons

We also like to remove the context menu actions once the buttons are in place.

Please let us know if you have further questions, and thanks once again for working on this issue.

nierajsingh commented 10 years ago

As a follow-up, for our case we do not need a "Select" button. Just:

Thanks.

Phanatic commented 10 years ago

@nierajsingh, thanks for the feedback, I'll make the changes to closely resemble the screen you have above.

Phanatic commented 10 years ago

Updated the code to change the layout of the window and removed the Contenxt Menu actions, please review. Environment variable edit window

nierajsingh commented 10 years ago

Thanks very much for doing this work. The UI looks like what we were expecting, except maybe reduce the spacing between the buttons, if you don't mind making that change. I'll test the wizard in the next couple of days and follow up here. Thanks very much for all the effort.

Phanatic commented 10 years ago

Thanks nieraj, I'll fix the padding between the buttons and push a change later today.

Phani

On Jul 24, 2014, at 9:23 AM, nierajsingh notifications@github.com wrote:

Thanks very much for doing this work. The UI looks like what we were expecting, except maybe reduce the spacing between the buttons, if you don't mind making that change. I'll test the wizard in the next couple of days and follow up here. Thanks very much for all the effort.

— Reply to this email directly or view it on GitHub.

elsony commented 10 years ago

The new changes looks good to me. One minor thing is to change the labels from "New" to "New..." and "Edit" to "Edit..." (and leave "Remove" as is) to follow the Eclipse UI convention.

Phanatic commented 10 years ago

Thanks @elsony, I made the suggested edits to the button captions.

nierajsingh commented 10 years ago

Hi,

I did further testing with the latest fix for persisting environment variable changes, and there is one more issue that would be nice to fix.

When you remove an environment variable, the Edit and Remove buttons still remain enabled even though there is no selection (as the selection was removed). It seems the listener that enables Edit and Remove button that gets registered in the table for SWT.Selection and SWT.FocusOut events does not get notified because clicking on "Remove" does not result in either event being fired, at least not on OS X.

Just to further showcase the problem, if I add the following:

table.addListener(SWT.FocusIn, actionEnabler);

and then in handleDelete() I do something like this:

    if (variableChanged) {
        envVariablesViewer.getTable().setFocus();
        notifyStatusChange(Status.OK_STATUS);
    }

Then the problem gets resolved, and the Edit and Remove button states are correctly refreshed. This is not necessarily an elegant solution, but if you can find a way to make sure that when "Remove" is pressed, the button states are refreshed, that would be great. Thanks for doing the work.

Phanatic commented 10 years ago

Thanks for the feedback @nierajsingh , I'll fix the enable/disable logic to correctly handle the edit/remove of an entry