RoboZonky / natural-strategy-setup

Webový konfigurátor investiční strategie pro RoboZonky
https://robozonky.github.io/konfigurace-strategie/
2 stars 1 forks source link

Implement possibility to save / load strategy config for later editing #35

Closed jhrcek closed 6 years ago

jhrcek commented 6 years ago

TODO find out if it's possible to process file "upload" via client side only.

jhrcek commented 6 years ago

@triceo I've got some rough implementation of saving / restoring state using local storage and it works nice in chrome and firefox (for now ignore the placement, missing tooltips, missing confirmation dialogs etc). "Obnovit strategii" button appears after you save and refresh the page. Let me dump some thoughts / worries for later discussion.

I can think of three major ways to persist the state of the tool's form (supposing we want to keep things simple and remain client-side only):

1. Local storage

The tool would have a bunch of buttons for managing saving / loading of the form state.

2. Downloadable file with form state

There would be two buttons

3. Shareable URL

triceo commented 6 years ago

@jhrcek I think I like 3 the most - it seems to be the simplest and universally accessible. The URL to the strategy could be put to the strategy itself as a comment, so people needn't remember it or write it down. Also, the strategy URL probably wouldn't be extremely long if we chose to encode it with something like Base64. (But yes, it will still be long.)

One thing we need to solve is the versioning business - how do we tell which migrations to apply to a strategy file to move it to the latest version supported by the tool?

triceo commented 6 years ago

(For future reference: https://rfc.zeromq.org/spec:32/Z85/ is essentially "Base64" for even more space efficiency.)

jhrcek commented 6 years ago

The more I think about it, the more I like option 3 too. Also I like your idea of putting the comment with URL to strategy config itself :+1: I'll try this approach next and see where it gets me.

Regarding strategy versioning and future changes of format: I'll simply provide a function for migration of each new version of strategy like this

jhrcek commented 6 years ago

I looked at Base85 and it doesn't seem to be suitable for putting into URL (there are only 73 valid characters that can be put into URL without escaping). But let's worry about space efficiency later, after I've got first working implementation :-)

jhrcek commented 6 years ago

@triceo here is a rough sketch of form state serialized in URL. More work is required (appropriate error reporting when deserialization from URL fails, expand the text area so the config preview fits etc.), but the core logic is there. The url is sometimes veery long. Also for some reason it doesn't work when I put the comment with url at the end (getting some exceptions from robozonky parser) so for now I put the comment with url in the beginning. Check it out and let's discuss it tomorrow.

jhrcek commented 6 years ago

Note for self: lz-string compresses the encoded strategy to about 1/3 length.

jhrcek commented 6 years ago

@triceo here is 2nd try based on optimized encoding of strategy config to json. The internal strategy data structure is encoded to json and then base 64 encoded into URL hash.

In the first implementation the average length of this hash was 7715 characters per strategy (using 1000 randomly generated testing strategies - note that those strategies are not realistic, as they have more filtering rules for better test coverage, than what users would normally put into strategy)

In this 2nd implementation the average length of the hash is 1908 characters per strategy (~24% of the original length).

After this I played with a javascript library for data compression, which reduces the length by further 25% on average (that is from 1908 to about 1450 characters). To be honest In my opinion the additional complexity introduced by calling external JS library is not worth the trouble and I'd prefer having simpler code than shorter url.

One more issue: I'd like to put the strategy comment at the end of the config, but I'm getting these

parser errors from Robozonky when doing that ``` java.lang.StringIndexOutOfBoundsException: String index out of range: 3272 at java.lang.String.(String.java:205) at org.antlr.v4.runtime.CodePointCharStream$CodePoint16BitCharStream.getText(CodePointCharStream.java:215) at org.antlr.v4.runtime.Lexer.notifyListeners(Lexer.java:360) at org.antlr.v4.runtime.Lexer.nextToken(Lexer.java:144) at org.antlr.v4.runtime.BufferedTokenStream.fetch(BufferedTokenStream.java:169) at org.antlr.v4.runtime.BufferedTokenStream.sync(BufferedTokenStream.java:152) at org.antlr.v4.runtime.BufferedTokenStream.consume(BufferedTokenStream.java:136) at org.antlr.v4.runtime.Parser.consume(Parser.java:571) at org.antlr.v4.runtime.Parser.match(Parser.java:203) at com.github.robozonky.strategy.natural.NaturalLanguageStrategyParser.sellMarketplaceFilter(NaturalLanguageStrategyParser.java:2587) at com.github.robozonky.strategy.natural.NaturalLanguageStrategyParser.sellFilterExpression(NaturalLanguageStrategyParser.java:2354) at com.github.robozonky.strategy.natural.NaturalLanguageStrategyParser.complexExpression(NaturalLanguageStrategyParser.java:552) at com.github.robozonky.strategy.natural.NaturalLanguageStrategyParser.primaryExpression(NaturalLanguageStrategyParser.java:315) at com.github.robozonky.strategy.natural.NaturalLanguageStrategyService.parseWithAntlr(NaturalLanguageStrategyService.java:86) at com.github.robozonky.strategy.natural.GeneratedStrategyVerifier.parseWithAntlr(GeneratedStrategyVerifier.java:27) at cz.janhrcek.nss.RandomStrategyRenderingTest.strategyIsParsableByRobozonky(RandomStrategyRenderingTest.java:56) at cz.janhrcek.nss.RandomStrategyRenderingTest.randomStrategiesCanBeParsed(RandomStrategyRenderingTest.java:33) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:252) at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:141) at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:112) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray(ReflectionUtils.java:189) at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:165) at org.apache.maven.surefire.booter.ProviderFactory.invokeProvider(ProviderFactory.java:85) at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:115) at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:75) ```

Looks like Robozonky doesn't accept comments at the end. Do you think it's worth changing parser to accept the comments at the end, or is it ok to have the URL comment at the beginning? I don't like it in the beginning, as each change of strategy potentially makes the url longer, which makes the rendered config text "jump" up and down all the time..

Please play with the 2nd version (which has JUST optimized json and no further compression) and tell me what you think. Ok like this or would you prefer 1/4 shorter url for the price of more complexity?

triceo commented 6 years ago

Looks good! I'll see what I can do about the parser.

triceo commented 6 years ago

@jhrcek Comments at the end of the file work. Tried it. In fact, there's no reason why they wouldn't - whatever is detected as a comment, it gets disregarded by the parser. Do you have an example of a strategy that doesn't work?

jhrcek commented 6 years ago

Comments at the end of the file work

Indeed, you're right. I looked at the parser and noticed that the comment line HAS to end with a newline, which I didn't have the last time. So I moved the strategy comment to the end and redeployed. Let's discuss tomorrow if it's ready for deployment.

jhrcek commented 6 years ago

@triceo I think I'm satisfied with the current implementation, you can check it out here and if you like it I'll create PR to official robozonky site.

Changes from last time

triceo commented 6 years ago

LGTM @jhrcek.

jhrcek commented 6 years ago

Ok, created PR to robozonky repo https://github.com/RoboZonky/robozonky/pull/223