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

Port to electron + add new features #68

Merged
merged 16 commits into from
Jul 4, 2018
Merged

Port to electron + add new features #68

merged 16 commits into from
Jul 4, 2018

Conversation

blurymind
Copy link
Collaborator

@blurymind blurymind commented Jun 10, 2018

Hi, this is somewhat of a big change pull, so please bear with me

It ports Yarn over to electron, while also adding a couple of new features:

  • Ability to easily create links to other existing nodes via a new helper menu:

yarn-linkmaking
(addressing #34 )

  • Ability to test dialogue via e new "Run" menu (using bondage.js and bbcode)

yarn-testerbbcode
(addressing #29 )

I am not sure if the shift to electron would be welcome by the original creators:
@InfiniteAmmoInc , @seiyria and @beeglebug

Please let me know,so I know if a better route for me is to fork it to "Yarn-plus" or "Yarn-electron" or something

In order to try it just run
npm install
then
npm start

- Helper menu to make it easier to create node links
- Run menu to test dialogue (via bondage.js and bbcode)
Terminate the story tester window if yarn is closed
@addie-lombardo addie-lombardo self-requested a review June 11, 2018 19:32
@addie-lombardo addie-lombardo changed the base branch from master to electron June 11, 2018 20:34
Copy link
Collaborator

@addie-lombardo addie-lombardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start, for now we'll merge it into its own branch so we can run more testing before merging it into master.

Before doing so however I'm having issues on my end calling JavaScript code as demonstrated in the GIFs attached to the PR in addition to errors being thrown once
parsing error

@blurymind
Copy link
Collaborator Author

blurymind commented Jun 11, 2018

@charblar hi thank you for looking into this. Sorry I forgot to mention that bondage.js doesnt support running the syntax
<<function(parameters)>>
At the moment. I have submited another pull request to add that already here:
hylyh/bondage.js#37
And used the same gif to demonstrate it there

The author of bondage.js @jhayley will review it when she has time, but basically adds the feature.
Feel free to run the test with it and see if it works :)

I am going to work on this a bit more this week hopefully to clear up some corner cases where it spills an error upon completion and center the text window. The code can do with some cleaning up too :)
I have to fix the styling on one of the search filters

@addie-lombardo
Copy link
Collaborator

addie-lombardo commented Jun 12, 2018

@blurymind hey, no problem! I assumed that was the case. I personally know Hayley and will work with her in the coming days to resolve that.

Thank you for the clarification, looking forward to your additional commits!

Edit: Oops hit comment and close, damn you GitHub and your lack of confirmation messages.

This fixes the styling problems, also cleans up some of the code on the renderer.js front
@blurymind
Copy link
Collaborator Author

blurymind commented Jun 12, 2018

@charblar thank you for looking into that. It wasn't really intended to be a part of this pull request - and still isnt. I needed the javascript parsing capability for my personal project. So in my pull request here that is not supported, it's more of a feature for bondage.js and not yarn.

On the front of this one - I fixed the previous styling problems and console errors, while also cleaning up the code.Now it should work much better :)

I am now shifting my focus on this pull and will go back to bondage.js a bit later. @jhayley doesnt like the syntax <<jsfunction(parameters)>> , so I might have to change it to <<eval jsfunction(parameters)>> please join the discussion at the other git repo pull if you have suggestions/ideas :)

@blurymind
Copy link
Collaborator Author

blurymind commented Jun 13, 2018

Ok going back to bondage.js, my approach was not right, so I will create another pull request - possibly later in the week for adding support to do <<command(parameters)>> in bondage.js.
I misunderstood the intended design of <<>> and the problem, so scrapped it and will redo that to allow <<function(parameters)>> syntax without introducing a new node type

For the commands - maybe later on I can add to the story tester the ability to show commands that were passed to it as well. I have noticed that it was kind of requested before

Also the tester window now auto-closes when finished testing
@blurymind
Copy link
Collaborator Author

blurymind commented Jun 14, 2018

A little update on this- after reading some requests here I decided to add the option to also show commands like this:
yarnrunnerupdate1
Also made the window self close when finished

I have to re-enable some things and sort them out before porting is complete. I also want to add one more usability feature to Yarn

@addie-lombardo
Copy link
Collaborator

Awesome, thanks for keeping us updated. Looking forward to it.

re-enable previously stubbed file commands. Yarn-electron can now successfully do the rest of the open/save operations
@blurymind
Copy link
Collaborator Author

Another update on this- I re-enabled all the file commands now- so the electron version is on-par with the main one.
Please note that the command debugging depends on a pull request I made to bondage.js - so command debugging is not there yet, since that needs to be merged first

I also made some changes to my vscode start script to get it to run it from a portable flash thumb- if you are trying it- you might have to revert the changes in .vscode/tasks.json for now :)

Anyways, I hope to have an electron build ready for testing soon-ish

@hylyh
Copy link

hylyh commented Jun 15, 2018

As a note, the changes being made to bondage.js are largely unrelated to the electron change and i recommend separating these concerns. Those feature requests aren't needed to port yarn to electron

@blurymind
Copy link
Collaborator Author

blurymind commented Jun 16, 2018

@jhayley yes of course. I have been using the electron port for testing the pull I am doing to bondage.js

Just wanted to note that the electron port at the moment will have this feature broken for most

Edit:
I have removed it from the electron port

@blurymind
Copy link
Collaborator Author

blurymind commented Jun 20, 2018

A little update on this.
There is now an electron release- that removes the command debugging for now and uses the official bondage.js branch (@jhayley )
:)
You can download it here
https://github.com/blurymind/Yarn/releases/tag/untagged-1933231011a749959b56

@charblar removing command debugging resolves the change you requested :)

For next update I am going to implement mid-click panning and try to also make linux and mac builds.
Another feature I am adding is automatic updating via the electron framework

This was referenced Jun 22, 2018
This adds:
- file drag and drop (including support for dropping multiple files )
- Support for deleting multiple selected nodes (via the delete key)
- Fix spacebar key (should be on key release- so it doesnt repeat while held)
- new key binding- ctrl+space while editing a node will switch to edit the next node
- new key binding- ctrl+enter while editing a node will save and close the node
- run document format prettify
@blurymind
Copy link
Collaborator Author

blurymind commented Jun 22, 2018

A little update:

I added:

  • file drag and drop (including support for dropping multiple files )
    yarn-dropfile

dropping multiple files appends them:
yarn-dropfilesmany

  • Support for deleting multiple selected nodes (via the delete key)
  • Fix spacebar key (should be on key release- so it doesnt repeat while held)
  • new key binding- hitting enter (after switching to a node via spacebar) will open it
  • new key binding- ctrl+space while editing a node will switch to edit the next node
  • new key binding- ctrl+enter while editing a node will save and close the node
  • run document format prettify

@blurymind
Copy link
Collaborator Author

blurymind commented Jun 27, 2018

A little update today - I added another usability feature 👍
yarn-autonewnodefromchoice
:)
This addresses #27
I will merge the commit tonight

@blurymind
Copy link
Collaborator Author

blurymind commented Jun 27, 2018

@charblar this is shaping up to be an epic release and I have addressed your request - it is no longer spitting out an error :)
For now I am not going to add the command debugger to the tester

Can you tell me what else needs to be done to get this merged?

@julsam
Copy link
Contributor

julsam commented Jun 27, 2018

I really love the new features @blurymind +1 , but (and I don't want to sound like an ungrateful ***,) i wished you didn't run prettify... or at least not in the same commit as other changes.

@blurymind
Copy link
Collaborator Author

blurymind commented Jun 27, 2018

@julsam sorry- its a habbit from another project. I can undo that if @charblar requests it :)

@blurymind
Copy link
Collaborator Author

New build out - go grab a copy! :)
https://github.com/blurymind/Yarn/releases/tag/untagged-4a6d4d2b95bf71bdd264

@addie-lombardo
Copy link
Collaborator

@blurymind Hey, I was out of country the past week and am taking a look at all of this now. Thank you for you patience.

For the remainder of this PR I request that we hold off on features and changes outside the scope of porting to Electron.

@blurymind
Copy link
Collaborator Author

@charblar Hi no problem. I will stop adding any new features and focus on the porting.

From my tests the electron port seems to work fine. Can you please test and review it :) I hope I haven't introduced any regressions

@addie-lombardo
Copy link
Collaborator

@blurymind Looks good to me, I would ask that you first ignore the .vscode directory first and remove the existing files from inside of it and then we can merge it in.

@blurymind
Copy link
Collaborator Author

@blurymind Looks good to me, I would ask that you first ignore the .vscode directory first and remove the existing files from inside of it and then we can merge it in.

@charblar It is done now :)
Thank you for reviewing the changes.

@julsam
Copy link
Contributor

julsam commented Jul 4, 2018

Just to add more feedback: I've been using a modified version of @blurymind 's electron port for the last 2 weeks, I have not encounter any problems so far, it's very stable.

I've been using it only on windows though (win7 x64).

@blurymind
Copy link
Collaborator Author

@julsam thank you for taking the time to test it. I noticed that you even merged my changes to your fork (https://github.com/julsam/VineEditor) which is flattering 😊
After @charblar merges these changes I think of adding a few more small but useful things in a separate pull.

@addie-lombardo addie-lombardo merged commit a7026a5 into InfiniteAmmoInc:electron Jul 4, 2018
Copy link
Collaborator

@addie-lombardo addie-lombardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, merging in.

@blurymind
Copy link
Collaborator Author

@charblar Thank you !! 🎉 😊 🎉 🍾

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.

None yet

4 participants