Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Populating classifier info view should be more intuitive #1748

Closed
lwrage opened this Issue Mar 15, 2019 · 21 comments

Comments

Projects
None yet
2 participants
@lwrage
Copy link
Contributor

lwrage commented Mar 15, 2019

Currently the classifier info view is filled automatically based on a classifier selected in the AADL navigator.

It would be better to fill it based on a user command:

Remove automatic view population from classifier selection in navigator and replace it with

  • Context menu in navigator with classifier selected
  • Context menu in outline view with classifier selected
  • Context menu in editor when classifier name is highlighted (Do we know how to do that?)
  • Define a shortcut, e.g., F4 as in Open type hierarchy for Java

@lwrage lwrage changed the title Populating classifier info view is not intuitive Populating classifier info view should be more intuitive Mar 15, 2019

@lwrage lwrage added next and removed backlog labels Mar 17, 2019

@lwrage lwrage added this to the 2.4.1 milestone Mar 17, 2019

@AaronGreenhouse AaronGreenhouse added in progress and removed next labels Mar 19, 2019

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 19, 2019

This is actually harder than just reacting to the selection because you need to be able to

  • make the classifier info view active
  • communicate the classifier to show to the view

I am looking at org.eclipse.jdt.ui.actions.OpenTypeHierarchyAction to study the situation. Also relevant is org.eclipse.jdt.internal.ui.util.OpenTypeHierarchyUtil.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 19, 2019

Will be able to get rid of the synchronize/link with view functionality.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 19, 2019

Added action that appears in the AADL Navigator and the Outline views.

Doesn't do anything yet, just hooked in an action.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 19, 2019

Trying to get an action into the context menu for the AADL editor.

Found this in org.osate.xtext.aadl2.ui

	<extension
		point="org.eclipse.ui.commands">
		<command
			description="Organize With Clauses"
			id="org.osate.xtext.aadl2.ui.organize.with"
			name="Organize With Clauses">
		</command>
	</extension>
    <extension point="org.eclipse.ui.menus">
         <menuContribution
		 allPopups="false"
         locationURI="popup:#TextEditorContext">
        <command commandId="org.osate.xtext.aadl2.ui.organize.with"
		 style="push">
         <visibleWhen checkEnabled="false">
               <reference
                     definitionId="org.osate.xtext.aadl2.Aadl2.Editor.opened">
               </reference>
         </visibleWhen>
        </command>
      </menuContribution> 
   </extension>
   	<extension
		point="org.eclipse.ui.handlers">
		<handler 
			class="org.osate.xtext.aadl2.ui.handlers.OrganizeWithHandler"
			commandId="org.osate.xtext.aadl2.ui.organize.with">
		</handler>
	</extension>

Hoping it will work for me too.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 19, 2019

Okay, I got the command into the editor pop up, but the enablement is needs to be dealt with, because obviously, the selected item (text) cannot be adapted to Classifier. Probably need to override isEnabled in the handler class, but I have a feeling it's not that straightforward.

Come back to this later, just wanted to get the command into the menu for now.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 21, 2019

Update the formerly empty command handler so that it makes the classifier information view active (and opens it if needed). This is the first step to giving it a new input.

This is easy to do, but took a while to find out how:

  • The basic code I took from org.eclipse.jdt.internal.ui.util.OpenTypeHiearchyUtil.openInViewPart()
  • It took a while to figure out how to get the IWorkbenchWindow reference in the command handler. I knew it had to come out of the ExecutionEvent parameter in execute, but I coudln't figure out how to get. Eventually I found the HandlerUtil class. Use the static method HandlerUtil.getCurrentWorkbenchWindow(). You can also get the current selection using methods in HandlerUtil.
@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 21, 2019

NOTE: look atOrganizeWithHandler for ideas about how to deal with command activation from the text editor.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 21, 2019

Updated OpenInClassifierInfoViewHandler to process the current selection.

Need to update the view to take the input now. Also get to rip out all the link to editor stuff and the local selection handler.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 21, 2019

Hmm, my command seems to be active for nodes that aren't Classifier nodes. need to look into that later.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 22, 2019

Updated the handler to update the classifier information view input.

This was not working reliably at first. The handler was not getting the most up to date selection form the Outline or navigator views. This was because I had failed to implemented ClassifierInfoView.setFocus() property. It had the default null implementation. It is expected to forward the focus to one of the view controls. Not doing so messed up the state of the workbench it seems. I have fixed this to forward the focus to the member tree of the classifier info view. Now things works okay.

Still to do

  • Fix the command activation so that it only activates for Classifiers. I thought I did this before but it's not working correctly.
  • Make this work from within the text editor.
@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 22, 2019

Should also add context menu to the ClassifierInfoView that allows refocusing on a selected classifier.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 22, 2019

  • Fix the command activation so that it only activates for Classifiers. I thought I did this before but it's not working correctly.

Works correctly in the Outline view. But enables for non Classifier objects in the AADL Navigator. (Correctly disabled when more than classifier is selected.)

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 22, 2019

  • Fix the command activation so that it only activates for Classifiers. I thought I did this before but it's not working correctly.

Works correctly in the Outline view. But enables for non Classifier objects in the AADL Navigator. (Correctly disabled when more than classifier is selected.)

Fixed this. The problem was in the plugin.xml in the definition of the handler. Had to make it

    <extension
          point="org.eclipse.ui.handlers">
       <handler
             class="org.osate.ui.handlers.OpenInClassifierInfoViewHandler"
             commandId="org.osate.ui.showInClassifierInfoView">
          <enabledWhen>
             <with
                   variable="selection">
                <and>
                   <count
                         value="1">
                   </count>
                   <iterate>
                      <or>
                      	 <!-- For the AADL Navigator View -->
                         <adapt
                               type="org.osate.aadl2.Classifier">
                            <!-- true -->
                         </adapt>

                      	 <!-- For the Outline View -->
                      	 <adapt type="org.eclipse.xtext.ui.editor.outline.impl.EObjectNode">
	                         <test
	                               property="org.osate.ui.superType"
	                               value="Classifier"
	                               forcePluginActivation="true"/>
	                     </adapt>
                      </or>
                   </iterate>
                </and>
             </with>
          </enabledWhen>
       </handler>

In particular, I had to add the adapt clause around the test clause. The test for superType only works on EObjectNode objects. If the current object was not an EObjectNode it was evaulating to true. Not sure why it would do this, but that is the fact. Wrapping it with the adapt causes a fail if the object is not an EObjectNode, and then we test the EObjectNode object for the super type.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 26, 2019

For the XText editor, I made it so that the command is active all the time. The text selection is converted to an EObject using SelectionHelper. The classifier is derived from the object by either trying to get the classifier from it, or by searching for a containing classifier.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 26, 2019

To do:

  • Add command to the view itself to refocus on one of th ancestor classifiers
  • Update docs
@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 26, 2019

Do we need a keyboard shortcut? if so, which one?

@lwrage

This comment has been minimized.

Copy link
Contributor Author

lwrage commented Mar 27, 2019

I don't think we should look for the containing classifier. I'd prefer if this works only if the classifier name is selected. Otherwise it should bring up a dialog that tells the user to select a classifier name.
No keyboard shortcut necessary.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 27, 2019

I don't think we should look for the containing classifier. I'd prefer if this works only if the classifier name is selected. Otherwise it should bring up a dialog that tells the user to select a classifier name.

It seems like getting the classifierinfo view connection to the XText editor to work the way we want is not very easy. I've been using SelectionHelper.getEObjectFromSelection() which relies on the xtext EObjectOffsetHelper class. But this method already tries to be too fancy for our needs. I don't see a straightforward way to determine if the selection if of a name reference node.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 27, 2019

Update the help text

@lwrage

This comment has been minimized.

Copy link
Contributor Author

lwrage commented Mar 28, 2019

I just realized that the selection stuff should be doable: Get the classifier as it is done now, find the nodes for its name in the node model and check if exactly these nodes are selected.

@AaronGreenhouse

This comment has been minimized.

Copy link
Contributor

AaronGreenhouse commented Mar 28, 2019

I just realized that the selection stuff should be doable: Get the classifier as it is done now, find the nodes for its name in the node model and check if exactly these nodes are selected.

Played with this this morning. Still difficult to get what we want. The parse tree really doesn't want to give up this information in the that we need. Not sure it's worth pursuing any further.

@wafflebot wafflebot bot removed the review label Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.