Bugzilla – Full Text Bug Listing
| Summary: | DOT Editor - Add mark occurrences support for dot attribute names and values | ||
|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | Tamas Miklossy <miklossy> |
| Component: | GEF DOT | Assignee: | Zoey Gerrit Prigge <eclipse> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | ||
| Version: | unspecified | ||
| Target Milestone: | 5.3.0 (2019-12) M3 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
| Attachments: | |||
Hi Tamás, I have started to implement this - however, I have noticed I might have misunderstood what you intended. Could you please confirm what you meant by "selects" the attribute? And perhaps attach a screenshot of "selecting" and highlighting node Id occurrences? Thank you! :) Hello Zoey, please find a screencast about selecting a nodeId and highlighting its all occurrences. That functionality has been implemented in https://bugs.eclipse.org/bugs/show_bug.cgi?id=530699, that issue contains more info about the Xtext Toggle Occurrences feature. Created attachment 279852 [details]
screencast about the current toggle occurrences highlighting
I pushed the following changes to the master branch. [548911] Mark occurrence support for attributes - Implemented mark occurrence support for Attribute Names and Values in DotOccurrenceComputer - Rewrote DotOccurrenceComputer to adapt logic to Dot and to be able to base occurrence matching on INode (EAttributes). - Added methods to find all matching attributes in DotAstHelper - Added parsed(Attribute) method in DotAttributes and generated helper method parsedAsAttribute to compare parsed attribute values. - adapted DotAttributeProcessor to generate parsedAsAttribute method - adapted DotMarkingOccurrencesTest to include helper method for strings longer than 1 character - implemented corresponding DotMarkingOccurrencesTest(s) - implemented corresponding DotAttributeActiveAnnotationTest to include parsedAsAttribute Tamás, your example should now be working. Resolving as fixed in 5.3.0 (2019-12) M2. Thanks for the fix, Zoey! Currently, if the attribute values are enclosed by quotes, they are also selected (see also the attached screencast). I think it would be better to select only the values without the quotes. Could you please adapt the current implementation? Thanks a lot in advance! Created attachment 280599 [details]
screencast about the current behaviour (quotes selection)
Currently the following exception is throw when deleting the content of a dot file: org.eclipse.emf.common.util.BasicEList$BasicIndexOutOfBoundsException: index=0, size=0 at org.eclipse.emf.common.util.BasicEList.get(BasicEList.java:346) at org.eclipse.gef.dot.internal.ui.language.markoccurrences.DotOccurrenceComputer$1.exec(DotOccurrenceComputer.java:77) at org.eclipse.gef.dot.internal.ui.language.markoccurrences.DotOccurrenceComputer$1.exec(DotOccurrenceComputer.java:1) at org.eclipse.xtext.util.concurrent.CancelableUnitOfWork.exec(CancelableUnitOfWork.java:26) at org.eclipse.xtext.resource.OutdatedStateManager.exec(OutdatedStateManager.java:91) at org.eclipse.xtext.ui.editor.model.XtextDocument$XtextDocumentLocker.internalReadOnly(XtextDocument.java:524) at org.eclipse.xtext.ui.editor.model.XtextDocument$XtextDocumentLocker.readOnly(XtextDocument.java:496) at org.eclipse.xtext.ui.editor.model.XtextDocument.readOnly(XtextDocument.java:135) at org.eclipse.gef.dot.internal.ui.language.markoccurrences.DotOccurrenceComputer.createAnnotationMap(DotOccurrenceComputer.java:65) at org.eclipse.xtext.ui.editor.occurrences.OccurrenceMarker$MarkOccurrenceJob.run(OccurrenceMarker.java:152) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63) See also the attached screencast. Therefore I change the status from RESOLVED FIXED to REOPENED. @Zoey: It would be nice if you could take a look at this problem. Thanks a lot in advance. Created attachment 280603 [details]
screencast reproducing the exception
Thanks for the info, Tamas. I will look into comment #7 with urgency. Regarding the quotes, we had decided to do the highlighting based on semantics. Hence, I am wondering if the quotes should be excluded as the quotes are part of the attribute value. (see comment #5) I will look into this once again in due course. Dear Tamas, I pushed a fix to the IndexOutOfBoundsException being thrown (see comment #7 and attachment #280603 [details]) into the master branch. This error should now be resolved. As promised, I will look into the other issue in due course. I have just tested the latest build and the problem does not occur any more. Thanks for the fix Zoey! Hi Tamás, I have revisited your request for adapting the implementation in comment #5. I believe we should refrain from doing so: Firstly, the current implementation is inline with our earlier decision to highlight according to semantics on the xtext model. You pointed out, it would be a beneficial aspect to allow the user to understand the model. Regardless, I have looked into the issue again - as we will agree usability is an important aspect. I also advised with a colleague regarding this. We both think that if the semantics of both '"text"' (with quotes) and 'text' (without quotes) are equal, the quotes should be included in the highlighting. This further seems standard practice in xtext and eclipse environments. Practical use cases arguing against the change would be: We agreed as a reason for our decision to do highlighting based on semantics that strings such as style="dotted, solid, bold" style="dotted,solid,bold" should both be highlighted if selected, even though they do not match character by character. These strings do not neither have any meaning without the enclosing quotation marks nor does it seem logical that the extra spaces should be hightlighted whilst the quotation marks aren't. Finally, if we do not highlight quotation marks, we would be unable to highlight empty ("") attribute values - which could be beneficial for some use cases. I would therefore hope you agree that it is a matter of personal preference. Therefore, I think we should factor implications for the implementations into this decision. Due to the choice to focus on semantics, I was able to eliminate if/switch statements and extract common highlighting function based on EStructuralFeatures. I personally think this allows for code which is easier to understand and could be reused at a later point. I do not think it would be a good idea to introduce a 'special case' on the provision that the benefit is doubtful. I hope you agree and are happy for me to close the bug as resolved fixed in 5.3.0 (2019-12) M3. =) I will attach two screenshots to illustrate the examples given. Created attachment 280806 [details]
Empty String Highlighting
Created attachment 280807 [details]
Multiple Styles Highlighting Example
Thanks for the detailed explanation! I agree with you, you can close the ticket again and resolved as fixed. Thanks, Tamás. Closed as resolved fixed in 5.3.0 (2019-12) M3 as per original implementation (comment #4), bugfix (comment #10) and agreement to not implement further changes (comment #15). |
Currently, the DOT editor provides support for marking the node Id occurrences. This support should be extended for attributes names and also for attribute values. Example: digraph { 1 [label=text] 2 [label="text"] 3 [label=text] 4 [label="text"] 5 [label=text] } - if the user selects the attribute name 'label', then all the other attributes with the name 'label' should be highlighted. - if the user selects the attribute value 'text', then all the other attributes with the value 'text' should be highlighted. The implementation of the DotOccurrenceComputer is located under: https://github.com/eclipse/gef/blob/master/org.eclipse.gef.dot.ui/src/org/eclipse/gef/dot/internal/ui/language/markoccurrences/DotOccurrenceComputer.java The corresponding test cases can be found under: https://github.com/eclipse/gef/blob/master/org.eclipse.gef.dot.tests/src/org/eclipse/gef/dot/tests/DotMarkingOccurrencesTest.xtend