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

Folding feature #846

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Folding feature #846

wants to merge 18 commits into from

Conversation

arichiardi
Copy link
Contributor

Hello Laurent,
this is the feature for folding, I tried to keep the commits small...

I have already noticed some issue, but please test if/when you can so that we can find more and fix them!

Thanks! 😄
Andrea.

This was referenced Aug 25, 2015
@arichiardi
Copy link
Contributor Author

I am adding some functional tests..things are still a bit rough..

@laurentpetit laurentpetit added this to the 0.35.0 milestone Aug 28, 2015
@laurentpetit
Copy link
Member

Hey, is it ready for a review, or is still a bit rough on the edge and you need still some time?

@arichiardi
Copy link
Contributor Author

I still need some time...

@arichiardi
Copy link
Contributor Author

Hello Laurent,
I think that if you feel like/have time, you can start reviewing this one. I still have to fix some stuff but I don't think I am going to change what is below the commit marked Rebasable/In Progress. It was pretty tough to put all the pieces together and I still have some glitch that I prefer to test so that I am not going to leave anything undocumented.
This is especially true for the dreaded #799 fix, which is hard to test directly (I don't think I can grab the background color of the AnnotationRulerColumn, there are many...it takes time to study), but easier to test through invariants on our ClojureEditor.
I also took heed of SWTBot's advice and implemented a FoldingBot. They basically say that it becomes easier you create a bot for each "functionality" to test that wrap common actions.
I am ready to answer to your comments promply 😄 😃
Go go Go !

Counterclockwise was not able to correctly identify some Clojure char token (\newline) as the
parsing was not adhering to the rules. By adapting parts of Clojure's 1.7 LispReader class, now CCW
can correctly identify __clojure_char partitions.

In the process, TokenScannerUtils has received some refactoring.
…Utils

Little cleanup of ClojureTokenScanner and TokenScannerUtils that refactors some reusable keywords
and removes the need of having state (utils usually are just static functions).
A init-text-buffer has been created in order to have an explicit creation point for documents.
…for editor

Given the new package structure, it makes more sense to have "/editor/scanners" and "/editor/text"
as tracing options instead of "/editor/scanners/partitioners".
This new package will contain everything related to text modification/strategies/ops.
The name better suits the purpose of this partitioner (which just wraps and trace partitions).
…ocol

The method getProjectionAnnotationModel has been added to IClojureEditor and consequently to
ClojureEditor as part of the folding functionality.
…anner

Little refactoring that brings IDocument.DEFAULT_CONTENT_TYPE in ClojurePartitionScanner.
@arichiardi
Copy link
Contributor Author

Laurent, I rebased on master and I get:`

Exception in thread "Thread-5" org.eclipse.e4.core.di.InjectionException: Unable to process "EventBroker.logger": no actual value was found for the argument "Logger".
    at org.eclipse.e4.core.internal.di.InjectorImpl.reportUnresolvedArgument(InjectorImpl.java:412)
    at org.eclipse.e4.core.internal.di.InjectorImpl.resolveRequestorArgs(InjectorImpl.java:403)
    at org.eclipse.e4.core.internal.di.InjectorImpl.inject(InjectorImpl.java:108)
    at org.eclipse.e4.core.internal.di.InjectorImpl.internalMake(InjectorImpl.java:333)
    at org.eclipse.e4.core.internal.di.InjectorImpl.make(InjectorImpl.java:254)
    at org.eclipse.e4.core.contexts.ContextInjectionFactory.make(ContextInjectionFactory.java:162)
    at org.eclipse.e4.ui.services.events.EventBrokerFactory.compute(EventBrokerFactory.java:29)
    at org.eclipse.e4.core.internal.contexts.ValueComputation.get(ValueComputation.java:61)
    at org.eclipse.e4.core.internal.contexts.EclipseContext.internalGet(EclipseContext.java:242)
    at org.eclipse.e4.core.internal.contexts.EclipseContext.get(EclipseContext.java:209)
    at org.eclipse.e4.core.internal.contexts.EclipseContext.get(EclipseContext.java:567)
    at ccw.CCWPlugin$2$1.run(CCWPlugin.java:348)
    at java.lang.Thread.run(Thread.java:745)

I remember having had some pain with this Logger. We will prolly need to push a Logger instance. see here

The support for folding Annotation/Position has been added through the following:
1) The implementation of ccw.editors.clojure.folding_support namespace which takes advantage of
paredit and its tree representation (embedded in the editor) for computing the folding positions and
then provides means to update the ProjectionAnnotationModel accordingly.
2) The reconciler, that includes a composite strategy containing the folding strategy in it. The
reconcile calls ccw.editors.clojure.folding_support/compute-folding! every round.This change
required a custom ClojureReconciler which handles part/editor hooks.
The patch adds a new page in the preferences and provides Java conversion from/to Clojure folding
descriptors (through the FoldingDescriptor/FoldingModel implementations).
This patch adds the necessary changes to perform tests for the folding feature. The test will go
into ClojureEditorTests.
…closure

The implementation of add-preference-listener changed in order to pass the PropertyChangeEvent to
the executed closure (also renamed to a more suitable f).

The hover-support.clj was the only one using this function and therefore the only one to be touched
by this patch.
This patch builds some harness for testing the correct opening/selection of Preference pages. It
also includes tests in SmokeTests.
New API introduced that create a org.eclipse.core.runtime.jobs.Job instance, mimicking
ccw.eclipse/workspace-job behavior.
The function ccw.eclipse/open-editors has been renamed to a better open-editor-refs as it returns
IEditorReference(s). Together with that, ccw.editors.clojure.editor-support/open-clojure-editors has
been added in order to return actual IClojureEditor instances.
This patch solves the dreaded issue #799 by adding better projection management on
IClojureEditor. It introduces an explicit enableProjection on IClojureEditor and a new preference:
PreferenceConstants/EDITOR_FOLDING_PROJECTION_ENABLED.  The trick for issue #799 was to add a
listener that triggers every time AbstractTextEditor/PREFERENCE_COLOR_BACKGROUND changes and turns
off and on the projection fixture on all the editors. By doing this, they paint their
projection/folding vertical column with the right color.
Following jdt's path, a FoldingActionGroup has been added to the editor. It will be shown when right
clicking on the folding column (the place where the expand/collaps buttons are shown).  Not all
jdt's features have been implemented, the missing ones are marked with TODO.
This patch adds more coherence tests for the folding feature. For instance, adds testing whether the
ProjectionAnnotationModel is empty when the preference options are all disabled.  The tests are in
FoldingTests and FoldingBot is the bot used for actions on the folding preference page.
@arichiardi
Copy link
Contributor Author

Actually, this link is better to see the issue.

@arichiardi
Copy link
Contributor Author

This feature is ready to review!

I will add ChangeLog and probably a section in our doc of course...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants