danieldietrich / xtext-protected-regions

Xtext Protected Regions
danieldietrich.net
12 stars 6 forks source link

Find better solution for JavaIoFileSystemAccess Wrapper Workaround #22

Closed danieldietrich closed 12 years ago

danieldietrich commented 12 years ago

BidiFileSystemAccess cannot be injected because Xtext wants a JavaIoFileSystemAccess.

danieldietrich commented 12 years ago

Cool! That makes sense!!

Btw.: minor question: what sounds better configurer or configurator?

ceefour commented 12 years ago

When Googling, seems like configurator wins. :) configurer seems to be a French word (?)

ceefour commented 12 years ago

The final architecture seems to be always like this:

  1. Wrapper JavaIoFileSystemAccess with PRS --> PRS --> BidiJavaIoFileSystemAccess
  2. Wrapper EclipseResourceFileSystemAccess with PRS --> PRS --> BidiEclipseResourceFileSystemAccess

At least with typical Xtext setup, the "wrapper"s are necessary. The wrappers act just like the Xtext bundled FSAs, but perform region merging transparently.

However the current wrapper as I implemented it is very simple, i.e. it does not support Bidi.

Although transparent merging makes PRS easy to use, maybe there are some generators who want to be "bidi-aware", ie it wants to read something in the FSA and perform custom logic based on it. So ideally, the outer wrapper should also support Bidi just like the inner BidiFSA.

danieldietrich commented 12 years ago

yes, sounds french - Monsieur le Configurèr :)

danieldietrich commented 12 years ago

Perhaps we can use composition instead of inheritence regarding AbstractProtectedRegionSupport and provide impls BidiJavaIo... + BidiEclipse... instead of the Wrapper. The Builder needs to buildJavaIoFSA() or buildEclipseResourceFSA() instead of build(). Having later time to go on with this idea...

ceefour commented 12 years ago

Sure! You're so much more skilled in API design.

I will open up a simple eCommerce sample project (issue #5) that everybody can observe and experience. Things get even more fun when "it's alive" (like Frankestein) :-)

danieldietrich commented 12 years ago

Cool - I like Frankenstein ;-)

danieldietrich commented 12 years ago

Hendy,

thought a while about it but still didn't get it: Why does Xtend depends on JavaIoFileSystemAccess?

Ok, the default generated Main class has a

@Inject 
private JavaIoFileSystemAccess fileAccess;

But this will do it also:

@Inject 
private IFileSystemAccess fileAccess;

// RuntimeModule
@Provides
public IFileSystemAccess createIFileSystemAccess() {
    return new IFileSystemAccess() {
        private IFileSystemAccess delegate = new JavaIoFileSystemAccess();
        @Override
        public void generateFile(String fileName, CharSequence contents) {
            delegate.generateFile(fileName, filter(contents));
        }
        @Override
        public void generateFile(String fileName, String slot, CharSequence contents) {
            delegate.generateFile(fileName, slot, filter(contents));
        }
        @Override
        public void deleteFile(String fileName) {
            delegate.deleteFile(fileName);
        }
        private CharSequence filter(CharSequence contents) {
            return contents; // TODO: manipulate contents
        }
    };
}

Am I s.th. missing?

ceefour commented 12 years ago

I don't think that's possible. IFileSystemAccess is only usable from a generator, ie "public API".

However, for Xtend to setup generation, it has to call at least setOutputPath, and this is only available in AbstractFileSystemAccess at minimum. I haven't tried configuring Guice to provide AbstractFSA yet,might just work.

For generation in Eclipse environment, it probably also uses methods in EclipseFSA class.. I haven't confirmed this. On Oct 7, 2011 6:32 AM, "Daniel Dietrich" < reply@reply.github.com> wrote:

Hendy,

thought a while about it but still didn't get it: Why does Xtend depends on JavaIoFileSystemAccess?

Ok, the default generated Main class has a

private JavaIoFileSystemAccess fileAccess;

But this will do it also:

private IFileSystemAccess fileAccess;

// RuntimeModule
@Provides
public IFileSystemAccess createIFileSystemAccess() {
return new IFileSystemAccess() {
private IFileSystemAccess delegate = new JavaIoFileSystemAccess();
@Override
public void generateFile(String fileName, CharSequence contents) {
delegate.generateFile(fileName, filter(contents));
}
@Override
public void generateFile(String fileName, String slot, CharSequence
contents) {
delegate.generateFile(fileName, slot, filter(contents));
}
@Override
public void deleteFile(String fileName) {
delegate.deleteFile(fileName);
}
private CharSequence filter(CharSequence contents) {
return contents; // TODO: manipulate contents
}
};
}

Am I s.th. missing?

Reply to this email directly or view it on GitHub:

https://github.com/danieldietrich/xtext-protectedregions/issues/22#issuecomment-2316371

ceefour commented 12 years ago

I don't generate using the Main class but using MWE2 program (also generated by Xtext).

I don't see any direct reference to JavaIoFSA in my Xtext project (saentity) so it seems the dependency is in the Xtext libs.

ceefour commented 12 years ago

BTW I tried @Provides AbstractFileSystemAccess and still doesn't work. It seems to explicitly depend to JavaIoFileSystemAccess. (and believe similar behavior for the Eclipse counterpart)

danieldietrich commented 12 years ago

All right - should be no problem. I redesigned our API and the Impl. I will stick it all together and provide it in a branch. Then we can test it. But next is bringing Emil, my son, to bed :-)

ceefour commented 12 years ago

I've checked the new API, I don't think this will work:

@Provides public JavaIoFileSystemAccess createJavaIoFileSystemAccess(IProtectedRegionSupport support) { IProtectedRegionSupport support = new ProtectedRegionSupport(); support.addParser(RegionParserFactory.createJavaParser(), ".java");

support.readRegions(.....);            // <---------- this can't be done

// support.setFilter(...); // (optional)
return new BidiJavaIoFileSystemAccess(support);

}

The reason is readRegions requires:

final URI uri = reader.getUri(path);
if (!reader.exists(uri)) {
  logger.warn("path does not exist: {}", uri.getPath());
  return;
}
if (!reader.hasFiles(uri)) {
  throw new IllegalArgumentException("no directory: " + path);
}
String canonicalPath = reader.getCanonicalPath(uri);
if (isVisited(canonicalPath)) {
  logger.warn("skipping already visited path '{}'.", path);
  return;
}
internal_read(reader, uri);

and all of those require that setOutputPath() already been called. However, we're just creating the FSA instance, not yet initializing it. That's why in my previous implementation, the initialization are deferred to the first actual FSA call (generateFile etc.) and that's when the configurator closure is executed.

Too bad Xtext don't have a lifecycle phase for this. (like JSF, or at least @PostConstruct)

IMHO the IPRS.readRegions() calls should schedule initializations, but not actually run them yet until it's "ready".

Please correct me if I'm wrong.

danieldietrich commented 12 years ago

Hi Hendy,

thank you four your review. In my opinion it should work with the redesign. I don't see the reason why support.readRegions(...) is any more called within the @Provides JavaIoFileSystemAccess.

Indeed, the new API requires, that the readRegions() method is not called directly by the User. Instead it is called indirectly by the BidiXxxFileSystemAccess.setOutputPath() methods.

Are you sure that it is not sufficient that regions are read/loaded when setOutputPath is called (programmatically or by the Xtext framework)??

Here is a snippet from the README:

Runtime Module Configuration

Configure Guice to provide JavaIoFileSystemAccess in your RuntimeModule class.

Example:

public class MyRuntimeModule extends AbstractMyRuntimeModule {

    @Provides
    public JavaIoFileSystemAccess createJavaIoFileSystemAccess(IProtectedRegionSupport support) {
        IProtectedRegionSupport support = new ProtectedRegionSupport();
        support.addParser(RegionParserFactory.createJavaParser(), ".java");
        // support.setFilter(...); // (optional)
        return new BidiJavaIoFileSystemAccess(support);
    }

}

Portable Generators

Your existing generator remains untouched.

override void doGenerate(Resource resource, IFileSystemAccess fsa) {

    fsa.setOutputPath(...); // now loads protected regions in the background(!)

    // Look ma! Portable generators! No "proprietary" APIs, simply Xtext!
    fsa.generateFile(fileName, '''...Xtend/Xpand template here...''')

}
danieldietrich commented 12 years ago

One more question:

Where have you called the fsa.setOutputPath(...) method before we created Xtext Protected Regions Support?

Thank you for your patience and your help!

ceefour commented 12 years ago

Wow, I see. I didn't realize that before. So no more readRegions()! That's much better.

I don't call setOutputPath() directly, but it's indirectly called by the MWE2 Workflow, relevant snippet:

var modelPath = "/home/ceefour/spaces/soluvas/com.soluvas.sample.ecommerce/src/com/soluvas/sample/ecommerce"

var targetDir = "/home/ceefour/spaces/soluvas/com.soluvas.sample.ecommerce/src-gen"
var webappDir = '/home/ceefour/spaces/soluvas/ecommerce-web/src/main/webapp'

...

component = org.eclipse.xtext.generator.GeneratorComponent {
    register = com.soluvas.saentity.SaentityStandaloneSetup {}
    slot = 'model'
    outlet = {
        path = targetDir
    }
    outlet = {
        outletName = 'webapp'
        path = webappDir
    }
}
danieldietrich commented 12 years ago

Great. Thx for the fix! Then I will merge the branch back to the master. It seems that we're close to v1.0. What do you think... Perhaps at first we should work one or two weeks with that version before tagging it to 1.0.0-Release. If you have ideas if something is missing, let me know.

ceefour commented 12 years ago

After fixing the swapped slot bug,I confirm that the new API indeed works very well.

Test branch: https://github.com/soluvas/saentity/tree/redesign

Feel free to merge it back to master, Master! :-)

danieldietrich commented 12 years ago

fasten your seatbelts - masta of desasta is mergin in a minute :-)

danieldietrich commented 12 years ago

btw - the log output is very helpful now when starting unit tests - thanks for that!!!

danieldietrich commented 12 years ago

merged to master.