Various bug fixes & improvements #237

FaultyFunctions · 2021-01-19T00:20:07Z

Full changes:

Editor:

Rename Complete Tags to Auto Close Tags where appropriate.
Rename Complete Words to Autocomplete Suggestions where appropriate.
Rename Complete Closing Characters to Auto Close Brackets where appropriate.
Fix bug where the three tags mentioned above wouldn't toggle properly.
Fix how the Auto Close Tags works and remove unnecessary spaces.
Removed the auto close tag for [[ answer: | ]]. Not every one follows this format in their Yarn files and this will be depreciated anyway in the 2.0 Yarn spec. Same thing when clicking Add Link button.
Increased Add Link font a little to be more readable.
Added panning to selected node when you open the split editor.
Updated editor title style to accommodate really long node titles for desktop & mobile.
Split editor will now remember its previous width.

Canvas:

Hopefully fixed a bug where grid color didn't register sometimes? Need to find a better way to update this rather than setting a timeout.
Convert alignment alerts to Swal notifications to be less intrusive.
Fix bug where panning to nodes would break the canvas if not in-editor and split-editor is enabled.

Dialogs:

New file dialog will now ask for a file name after you confirm you want to open a new file.
Editing the new file dialog a bit to better match the rest of the app's dialog aesthetics.

Try it out and let me know if you find any problems! Thanks!

- The autocomplete and auto close buttons were a bit bugged. This fixes that to ensure they toggle correctly. - Renames autocomplete toggles and auto close buttons/settings to be more appropriate and understandable. - Fixes auto close tags to better work with the editor when ace editor behaviors is enabled. - Removed the auto close tag for [[ answer: | ]]. Not every one follows this format in their Yarn files and this will be depreciated anyway in the 2.0 Yarn spec.

- Fix for grid being a different color sometimes, maybe?

- Not everyone uses the [[answer:node|node]] format so this should be add [[node]] isntead.

- For better readability

- Less intrusive

- Will now ask for file name

FaultyFunctions · 2021-01-19T00:26:56Z

Also once this is merged and taken care of, I was wondering if we could get a new release so people can download the new changes? Thanks!

- Replaces them with sweet alerts instead. There is a bug where focus would be taken away from the electron app if alert() or confirm() was used. You had to then click off the app and then click back onto it to actually type stuff. This fixes it.

FaultyFunctions · 2021-01-21T00:46:00Z

Oops I maybe broke opening files on the web version. Investigating now...

EDIT: Fixed! + added an open file warning on the web version.

FaultyFunctions · 2021-01-21T08:02:38Z

YarnSpinnerTool/YarnEditor@000ab57 closes #219

- Change from NSIS installer to MSI installer for windows. This add automatic file associations and fixes the application icon not showing up, as I couldn't get these to work with the NSIS installer. The start menu icon still doesn't show up for me? Not sure why. - Double clicking .yarn files will now open the editor with the specified file opened, instead of just opening the previously edited file.

blurymind · 2021-01-22T01:06:48Z

Also once this is merged and taken care of, I was wondering if we could get a new release so people can download the new changes? Thanks!

Releases stopped coming because the electron automatic builds stopped working. I welcome a pr to fix them. Maybe @daviddq can help?

blurymind · 2021-01-22T01:12:23Z

I am against removing the autoclose tag for [[ or anything deprecating the old spec.
It is the only way to make a linked node for bondagejs users. Please keep compatibility with the old yarn until we can spin spec2 with a js module.

If having it steps on your usability, perhaps make spec2 optional instead of enforcing it onto existing users who can't use it?

I honestly fail to understand why not having backwards compatibility is not beneficial , especially when in this case you are literally breaking stuff for other users (me included)

I am a contributor like you and others. I contribute time to review prs. Ones that are sort of removing things I use, I will leave to someone else to review.

I am not trying to halt progress. On the contrary I love your work and the features you added. I will not try to stop you. If someone else approves it, that fine by me :) just might kill my interest in the branch a bit

FaultyFunctions · 2021-01-22T01:55:24Z

Oh sorry there has been a misunderstanding. I just removed the full autocompletion of it completing [[ to [[ answer: | ]] since not everyone uses the full format and some people want to use just [[node]]. This doesn't remove support for the [[ tag at all, it merely just doesn't pick one format over the other. This makes it less annoying for people who still want to use auto close tags for [b][/b] but don't want to get [[ answer: | ]] every time they type [[, if you understand what I mean.

blurymind · 2021-01-22T01:57:28Z

Ah phew you gave me a scare for a moment. I am fine with that then :)

FaultyFunctions · 2021-01-22T02:10:52Z

Also once this is merged and taken care of, I was wondering if we could get a new release so people can download the new changes? Thanks!

Releases stopped coming because the electron automatic builds stopped working. I welcome a pr to fix them. Maybe @daviddq can help?

I have no idea how the auto build stuff works or the dependabot thing functions. So any insight into that would be appreciated.

- You can now save while in-editor, and a nice save notification will pop up. - Also adds the missing tooltips for undo, redo, and create node.

FaultyFunctions · 2021-01-22T05:59:05Z

c58fe89 fixes #218

…n split view

- Now you can type a bunch of stuff in one node, then type a bunch of stuff in another node, and then go back to the first node and press Ctrl+Z. It won't delete the entire content of the node anymore, it'll undo just like if you have just typed the text.

FaultyFunctions · 2021-01-22T08:39:09Z

With d9fcd25 this can open up the possibility in the future of saving the entirety of the undo history in a file between sessions. So if you save a file and exit the editor, then open it back up again, you can still press Ctrl+Z and it'll undo text like you had just typed it. However, that's future work for a later time!

daviddq · 2021-01-22T08:41:52Z

Also once this is merged and taken care of, I was wondering if we could get a new release so people can download the new changes? Thanks!

Releases stopped coming because the electron automatic builds stopped working. I welcome a pr to fix them. Maybe @daviddq can help?

I will take a look next weekend.

blurymind · 2021-01-22T17:14:49Z

@daviddq thank you :)

With d9fcd25 this can open up the possibility in the future of saving the entirety of the undo history in a file between sessions. So if you save a file and exit the editor, then open it back up again, you can still press Ctrl+Z and it'll undo text like you had just typed it. However, that's future work for a later time!

cool :) it looks like the beginning of an undo manager. Would the file history be stored inside the file or another hidden file mirroring it or in localStorage cache (myFilePath>undoHistory...)? How will that work with gist hosted files?

Anyways, please keep big features like this for a separate PR. In fact if this pr is not using it to fix or do anything in any way, I would recommend removing it from the pr. Please no dead code in Prs

FaultyFunctions · 2021-01-22T17:21:03Z

Oh this data isn't stored in between sessions anywhere yet. It doesn't affect anything currently, it just keeps undo history internally while moving in between nodes. Once you exit the editor the history is reset just like it is currently. The way this works is by adding this.undoManager = null; variable inside node.js. Then when you exit the currently edited node it saves Ace's UndoManager object inside this variable, then when you load the node back up for editing it re-applies this UndoManager object to the editor.

That comment about saving the history was just speculation on what we can do with this in the future, otherwise this feature is mainly just to fix #37.

We can open another PR later or issue to discuss how this should work in full if we do decide to tackle actually saving undo history on the file.

blurymind · 2021-01-22T17:40:04Z

electron/package.json

-    "productName": "Yarn Editor"
+    "productName": "Yarn Editor",
+    "fileAssociations": {
+      "ext": "yarn",


ah, thats very nice :) will let the electron version open yarn files via the OS file manager?
I was wondering if we can do something for the pwa version of yarn

After some research it seems only Windows 10 Universal Apps and Chrome Apps have a file handling API for this. This is being worked on I think for regular PWA apps?
See:
https://www.reddit.com/r/PWA/comments/gol8jh/open_with_pwa/

Not sure if this is possible or not with out current set up.

There is a new api that gives access to the file system now
https://web.dev/file-system-access/
but there is a second part to this - how do we get the path to the file that is opened with the installed pwa?

This one might be a can of worms for another pr i guess :)

I assume something to do with the service worker and lots of testing required to implement properly

blurymind · 2021-01-22T18:14:45Z

src/index.html

@@ -52,18 +52,18 @@
    </div>	
    <!-- undo/redo controls -->	
    <div class="app-undo-redo app-button">	
-      <span data-bind="click: function() { app.historyDirection('undo'); }">
+      <span data-bind="click: function() { app.historyDirection('undo'); }" title="Undo">


nice :) I appreciate the tooltips

blurymind · 2021-01-22T18:16:19Z

src/js/classes/app.js

@@ -374,7 +383,10 @@ export var App = function(name, version) {
    this.hearText = function() {
      const available = spoken.listen.available();
      if (!available) {
-        alert('Speech recognition not avaiilable!');
+        Swal.fire({


nice, thanks for getting rid of the alerts

blurymind · 2021-01-22T18:24:37Z

src/js/classes/app.js

@@ -17,6 +17,7 @@ import { data } from './data';
 import { yarnRender } from './renderer';
 import { Utils, FILETYPE } from './utils';
 import { RichTextFormatter } from './richTextFormatter';
+import Swal from 'sweetalert2';


I wonder if this would work without importing it here. We are already importing it in index.js 🤔

blurymind · 2021-01-22T18:25:52Z

thanks for this, the code looks alright 👍 just need to test it for regressions

Edit: actually i left a few notes, mostly minor ones

blurymind · 2021-01-22T19:25:33Z

electron/yarn-main.js

+window.addEventListener("yarnReadyToLoad", e => {
+  yarn = e;
+  // Open file on doubleclick on Windows
+  if (remote.process.platform === 'win32' && remote.process.argv.length >= 2 && !isDev) {


this is a bit flaky. electron has a better way of dealing with this

EDIT:
if you are only checking for win32, what about other OS? You need to be checking if it is running on electron instead, which we are already. I say remove the platform check and not sure why also !isDev - unless you found running it in dev starts it with extra arguments

if (remote.process.argv.length >= 2)

Running it on dev makes remote.process.argv return an array with like 20 entries that doesn't contain any information we care about so without the isDev this would trigger and error out.

I wasn't sure how to get this working on the other OS systems. What's flaky about this? What's the better electron way? I tried multiple solutions but I could only get this one working... :/ If you wanna take a crack at it, I'd be more than happy!

This should be handled by electrons api, so you really shouldn't be limiting it just to windows users, even if you can't test it.
Keep the isDev check, but get rid of the win32 one.
If it doesn't work on Linux or mac, someone using those will submit a pr anyways

Ok so, instead of doing this in yarn-main, why dont you do it in electron/main.js?
You can detect if dom is ready
https://www.electronjs.org/docs/api/web-contents#event-dom-ready

then you can pass the args with
mainWindow.webContents.send(...

alternatively, why do you even have this here at all and not handle it all in data.js in a simpler and less loopy way?
if (app.electron?.remote?.process?.argv?.length >= 2)

This yarn-main file is an interface for bundling yarn in alien electron apps. When we are handling our own electron code, I prefer to add to main.js. That is really the entry point interface for the electron app that is built.

so 3 things:

remove the yarnReadyToLoad signal

handle this in one location, the code can be much simpler than this

do not put electron logic in this interface file. Use main.js instead. This file is for bundling yarn editor in iframes mostly :)

blurymind · 2021-01-22T19:26:55Z

src/js/classes/data.js

-    app.refreshWindowTitle();
+    })
+  },
+  openFileOnStart: function(path, filename) {


call this openFileFromLocalPath. A more descriptive and generic name is better imo. Might come in handy :)

blurymind · 2021-01-22T19:37:10Z

src/js/classes/app.js

@@ -144,7 +147,13 @@ export var App = function(name, version) {
      return null;
    });
    window.addEventListener('DOMContentLoaded', e => {
-      this.data.loadAppStateFromLocalStorage();
+      if (self.isElectron) {
+        var event = new CustomEvent('yarnReadyToLoad');


I believe we already have something to detect this event, so no need to create another

see how I used it for bundling in other electron apps
https://github.com/4ian/GDevelop/blob/ce5282f41b6ff0eafbd3063b20237e88d9262596/newIDE/app/public/external/yarn/yarn-main.js#L24

its called 'yarnReady'

Hmm I don't remember why I needed to create a new one but I did try using the "yarnReady" one and ran into some issues. I'll try to update this and see what happens, maybe I just overlooked something.

Hmm strange. What was the error? Why didn't you fix it there rather than creating a second event doing the same thing?

blurymind · 2021-01-22T19:40:24Z

src/js/classes/data.js

+            title: 'Unknown filetype!',
+            icon: 'error'
+          });
+          self.loadAppStateFromLocalStorage();


if it fails to load the file on start and loads a previous one - would that be a bit confusing? The user will think it loaded a different file than the one they double clicked on. I would remove this call to loadAppStateFromLocalStorage

blurymind · 2021-01-22T19:42:30Z

src/js/classes/data.js

+          self.loadAppStateFromLocalStorage();
+        } else {
+          data.editingName(filename.replace(/^.*[\\\/]/, ''));
+          data.editingPath(path);


I wonder if you can call data.openFile here instead of replicating these steps below

blurymind · 2021-01-22T19:46:24Z

electron/package.json

+      "name": "Yarn File",
+      "role": "editor"
+    },
+    "win": {


can you add icon for mac and linux too?

blurymind · 2021-01-22T19:55:16Z

src/js/classes/data.js

@@ -1,6 +1,7 @@
 /* eslint-disable jquery/no-ajax */
 const path = require('path');
 const saveAs = require('file-saver');
+import Swal from 'sweetalert2';


again, I dont think you need to import it here :)

src/js/classes/richTextFormatterHtml.js

blurymind · 2021-01-24T11:24:40Z

src/js/classes/app.js

@@ -90,6 +91,7 @@ export var App = function(name, version) {
  this.editingPath = ko.observable(null);
  this.$searchField = $('.search-field');
  this.isEditorInPreviewMode = false;
+  this.isEditorInPlayMode = false;


I wonder if these should be in the ui class

blurymind · 2021-01-24T11:45:12Z

src/js/classes/app.js

  // TODO: move to UI?
  this.togglePreviewMode = function(previewModeOverwrite) {
    const editor = $('.editor')[0];
    const editorPreviewer = $('#editor-preview')[0];

    self.isEditorInPreviewMode = previewModeOverwrite;
    if (previewModeOverwrite) {
-      self.togglePlayMode(false);
+      if (self.isEditorInPlayMode) {


I wonder if we should move these ui related variables to the ui class.. isEditorInPlayMode, isEditorInPreviewMode?

blurymind · 2021-01-24T11:49:40Z

Left some review notes :) The PR will actually break the app restore state feature on electron, so that needs to be remedied for sure before it moves forward

FaultyFunctions · 2021-01-24T12:00:07Z

Actually it doesn't break local storage on electron though. Check yarn-main.js. It has code specifically to load from local storage if no arguments are passed (a user didn't double click a file to open).

I just disabled it specifically in app.js because I didn't want it to load from storage and then after a second load from the file the user double clicked. Which was a bit jarring. So this way it just loads the file the user clicked right away and still uses local storage if no arguments are passed.

I'll check out your other comments soon!

blurymind · 2021-01-25T19:16:24Z

electron/yarn-main.js

+    let filename = path.basename(filepath);
+    yarn.app.data.openFileOnStart(filepath, filename);
+  } else {
+    yarn.app.data.loadAppStateFromLocalStorage();


ah I noticed it now - it would be much better if this conditional (if/else) split is handled in one location instead of two. Either in main.js or in the data class. I feel this is the wrong place to do this

blurymind · 2021-01-25T19:18:57Z

Actually it doesn't break local storage on electron though. Check yarn-main.js. It has code specifically to load from local storage if no arguments are passed (a user didn't double click a file to open).

I just disabled it specifically in app.js because I didn't want it to load from storage and then after a second load from the file the user double clicked. Which was a bit jarring. So this way it just loads the file the user clicked right away and still uses local storage if no arguments are passed.

I'll check out your other comments soon!

Would it be possible to do the if/else in either main.js or in data, but not both and not in yarn-main? When you split the logic across two files it makes the code harder to maintain. The other thing is - can you in the feature do a separate pr for each fix? When you lump them up together like this, its very hard to catch regressions and harder to follow.Its easier for me to miss something when the diff becomes huge :)

blurymind · 2021-01-25T19:21:02Z

Also once this is merged and taken care of, I was wondering if we could get a new release so people can download the new changes? Thanks!

Releases stopped coming because the electron automatic builds stopped working. I welcome a pr to fix them. Maybe @daviddq can help?

I will take a look next weekend.

@daviddq thank you for the awesome and really quick fix 🎉 it worked like a charm. We now have fresh electron builds, straight out of the oven 🍞 😄

blurymind · 2021-02-07T16:29:38Z

I reverted your implementation of the file loading from args and implemented a simpler approach (less loopy).

With that I think the PR is ok.

Please in the future dont bundle fixes in one PR.
Do a PR per fix - not several in one - that way they will be faster to review and merge!

FaultyFunctions added 10 commits January 18, 2021 14:59

Give grid a little more time to load

42aee52

- Fix for grid being a different color sometimes, maybe?

Remove extra stuff add link button adds

5fca37b

- Not everyone uses the [[answer:node|node]] format so this should be add [[node]] isntead.

Increase "add link" font size a bit

a163781

- For better readability

Add warping to node when opening split editor

cc19134

Update editor title style to accommodate really long node titles

27ab14e

Split editor will now remember its previous width

e8ed2eb

Convert alignment alerts to swal notifications

b706e78

- Less intrusive

Improvements to new file dialog

e23d6ef

- Will now ask for file name

Fix bug where panning to nodes would break if not in-editor

318abc1

Fix opening files on web version + add open file warning on web version

99b6c8b

Enable save in-editor + add saved notification + fix missing tooltips

c58fe89

- You can now save while in-editor, and a nice save notification will pop up. - Also adds the missing tooltips for undo, redo, and create node.

Fix blurymind#230 + playback mode will warp you to the current node i…

0978040

…n split view

FaultyFunctions mentioned this pull request Jan 22, 2021

Unity not detecting updated text unless I replace the old files. #211

Closed

blurymind reviewed Jan 22, 2021

View reviewed changes

Revert commit to push to different branch.

b82b0ed

blurymind reviewed Jan 22, 2021

View reviewed changes

FaultyFunctions mentioned this pull request Jan 22, 2021

Compatibility issue with Unity (Yarn Editor ver. 4.94/Unity ver. 2019.4.8f1 Personal) #209

Open

blurymind reviewed Jan 22, 2021

View reviewed changes

src/js/classes/richTextFormatterHtml.js Show resolved Hide resolved

blurymind reviewed Jan 24, 2021

View reviewed changes

blurymind reviewed Jan 25, 2021

View reviewed changes

blurymind added 4 commits February 7, 2021 12:40

partially revert commit

0616119

run format

a91ad42

implement loading of files from args on electron

c3f2f6d

remove redundant imports

df59f64

blurymind merged commit 8086682 into blurymind:master Feb 7, 2021

blurymind added a commit that referenced this pull request Feb 8, 2021

fix target:msi breaking windows builds (regression from #237)

c732621

FaultyFunctions deleted the various-bug-fixes branch February 9, 2021 05:14

Various bug fixes & improvements #237

Various bug fixes & improvements #237

Conversation

FaultyFunctions commented Jan 19, 2021 • edited Loading

FaultyFunctions commented Jan 19, 2021

FaultyFunctions commented Jan 21, 2021 • edited Loading

FaultyFunctions commented Jan 21, 2021

blurymind commented Jan 22, 2021

blurymind commented Jan 22, 2021 • edited Loading

FaultyFunctions commented Jan 22, 2021

blurymind commented Jan 22, 2021

FaultyFunctions commented Jan 22, 2021

FaultyFunctions commented Jan 22, 2021

FaultyFunctions commented Jan 22, 2021

daviddq commented Jan 22, 2021

blurymind commented Jan 22, 2021 • edited Loading

FaultyFunctions commented Jan 22, 2021 • edited Loading

blurymind Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blurymind Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blurymind Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

blurymind commented Jan 22, 2021 • edited Loading

blurymind Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blurymind Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

blurymind Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

blurymind Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blurymind Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

blurymind Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blurymind Jan 24, 2021 • edited Loading

Choose a reason for hiding this comment

blurymind commented Jan 24, 2021

FaultyFunctions commented Jan 24, 2021

blurymind Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

blurymind commented Jan 25, 2021 • edited Loading

blurymind commented Jan 25, 2021

blurymind commented Feb 7, 2021 • edited Loading

FaultyFunctions commented Jan 19, 2021 •

edited

Loading

FaultyFunctions commented Jan 21, 2021 •

edited

Loading

blurymind commented Jan 22, 2021 •

edited

Loading

blurymind commented Jan 22, 2021 •

edited

Loading

FaultyFunctions commented Jan 22, 2021 •

edited

Loading

blurymind Jan 22, 2021 •

edited

Loading

blurymind Jan 22, 2021 •

edited

Loading

blurymind Jan 22, 2021 •

edited

Loading

blurymind commented Jan 22, 2021 •

edited

Loading

blurymind Jan 22, 2021 •

edited

Loading

blurymind Jan 25, 2021 •

edited

Loading

blurymind Jan 22, 2021 •

edited

Loading

blurymind Jan 22, 2021 •

edited

Loading

blurymind Jan 22, 2021 •

edited

Loading

blurymind Jan 22, 2021 •

edited

Loading

blurymind Jan 24, 2021 •

edited

Loading

blurymind Jan 25, 2021 •

edited

Loading

blurymind commented Jan 25, 2021 •

edited

Loading

blurymind commented Feb 7, 2021 •

edited

Loading