thombruce / toodles

✅ A super simple todo app
https://toodles.thombruce.com/
GNU General Public License v3.0
0 stars 0 forks source link

[Bug]: 0.4.0-alpha.0 does not show document contents #131

Closed thombruce closed 4 weeks ago

thombruce commented 4 weeks ago

What happened?

This alpha build introduces the directory list, so we're now navigating to documents and... they aren't displaying anything.

A few things happen when we navigate to a document. We...

  1. Empty the list
  2. Set the document path in the store
  3. Obtain new todos for the list from the path

I suspect... the path we're setting/using is not valid.

The alpha is unusable in its current state, so this should be a priority fix.

Version

0.0.1 (Default)

What browsers are you seeing the problem on?

No response

Relevant log output

No response

Code of Conduct

thombruce commented 4 weeks ago

Potentially could have to do with the fact that I'm normalizing filepaths by default? Does Electron on Windows respect these normalized paths? If not, this is a fix that needs to be implemented on TNT. If it does, then the problem is probably here on Toodles.

The dev version does work in a WSL Linux environment, so... Ah, but that doesn't use the PORTABLE env variable. Maybe the issue is there? Perhaps I omitted an instance of that variable or... or... and here's what I most suspect...

Because I'm navigating to the file.path, it is this that is incorrect. I suspect we're using full paths when what we want are relative paths to the PORTABLE installation directory.

thombruce commented 4 weeks ago

Hmm, whatever the issue there are no errors raised in the console. I tried adding a todo to see if it persisted, and it did not. That... should really result in an error I can digest.

So... it thinks it's pointing to a document, but no document actually exists... at least... one doesn't persist. So maybe it's falling back on the old temp folder? In which case, this is definitely a matter of my not properly invoking the PORTABLE path somewhere.

thombruce commented 4 weeks ago

Maybe push a build that displays the path or a pre with full contents... so we can get an idea of what's actually going on.

Or maybe don't push a build and give building the app a whirl on Windows. Maybe I can find the issue that way.

thombruce commented 4 weeks ago

I've been thinking about canary builds, but if I'm going to break something this critical on main, I can't offer canary builds yet.

We need something pre-canary, then. We need to test our targets (mainly Windows right now) before faults like this get into the main branch. If we've done that, then we can offer canary builds. Builds that may have breaking changes, but not critically breaking ones like this.

thombruce commented 4 weeks ago

I can confirm the problem is this:

<a href="#/C:/Users/thom/Documents/Notes/_private.todo" class="">_private.todo</a>

What that path should look like is #/_private.todo. Instead it's the full path to the document on the file system.

On top of that, when we go to read or save from this fille path, we'll actually be prepending it with the PORTABLE env var, so we probably get something like... C:/Users/thom/Documents/Notes/C:/Users/thom/Documents/Notes/_private.todo.

So this is maybe a blessing and a curse, because on one hand... oh awesome, we can obtain full paths and presumably use these to look up the files on the filesystem, but also... if we say trim that route down to the part we're interested in, like by replacing the part that matches PORTABLE path with '', that's a choice we've made to focus portable only.

So maybe the solution is to... drop most instances of PORTABLE? Like the ones we use for looking up and saving to a file anyway... because we've got the full path now. So the question is, presuming that even would work, ought we to maintain an option for prepending paths with the PORTABLE executable dir? Like is that useful in cases where... we aren't otherwise yielding the folder path from this directory list?

I guess we could say, hey if this filepath string doesn't start with /^[A-Z]:/, we're gonna prepend it with the portable dir - we're gonna make that assumption. If it does start with that pattern, can we reasonably believe it to be a full path that will work for our reading/writing purposes?

On Linux, at least in dev, we seem to get a relative path anyway. On Windows, we're getting the full path. And maybe that's fine.

I guess try and push out a version that does the starts with check and prepends the PORTABLE dir on failure for the read/save functions. See if that works in the Windows Portable environment, and if it does... great! And bonus too: It's a step closer to non-portable support. And if it doesn't work... my next idea is annoyingly to trim the paths down somehow.

Documentation for saving files with Electron is oddly sparse.

thombruce commented 4 weeks ago

Here's a thought... File path format is gonna be different on Windows, Linux and Mac. Beyond which, a filepath like C:\somewhere is technically the same as but different to C:/somewhere.

With documentation seemingly so lacking, the only way for me to really find out if my implementation works on Windows is to... run it on Windows. The only way for me to investigate the results of various variables/functions on Windows is to output their results on Windows.

Nightmare.

The obvious solution originally was to pass a relativePaths: true argument or something like that to directoryTree but it doesn't look as though such an option exists. It is still the best idea - the ideal solution. I shouldn't rule it out - I'll investigate that further.

thombruce commented 4 weeks ago

Yeah, no luck on the directory-tree repo. The main dev doesn't run it on Windows at all, probably doesn't run it in Electron as I'm trying to do... so it's an untested, unsupported environment and... it's an edge case. I'm working in an edge case and have to solve this problem myself.

Second best solution then, I think, is to convert the paths I receive to relative paths. This requires the least rewriting of existing (and formerly working) logic. It is also the... most correct feeling solution for a portable app, and is the least disruptive in terms of also supporting Linux and Mac.

So here's my proposed solution:

  1. We set normalizePath to false for directoryTree (I had set it to true)
  2. We replace PORTABLE_EXECUTABLE_DIR in the :to link of our Nuxt links with '':
NuxtLink(v-else-if="links" :to="child.path.replace(process.env.PORTABLE_EXECUTABLE_DIR, '')" :key="child.path")  {{ child.name }}

If the string doesn't match the PORTABLE_EXECUTABLE_DIR anyway, nothing will happen, so it maintains whatever level of support we had with other platforms. If it does match it, then we're replacing it.

The result of this should be paths like... directory\subdirectory\todo.txt <- How much do I need to worry about these backslashes? I don't know.

But hey, that should work.

thombruce commented 4 weeks ago

Alright, here's a PR with my proposed solution over on TNT: https://github.com/thombruce/tnt/pull/87

Because TNT is a monorepo, I believe I still need to publish that before I can use it. I can't just update my package.json to target the repo... so... I'll publish an alpha. Makes sense. We'll bump it if it works.

thombruce commented 4 weeks ago

Version to test when built is now v0.4.0-alpha.1.

I'm somewhat optimistic about this working. If it doesn't... I have a few theories. Could be that the PORTABLE var does provide a normalized path and so doesn't match, could be the backslashes I worried about above, could be that the PORTABLE var isn't even available in the renderer process... I think it is, but it may not be. If it's not, that's a tougher problem to solve because I need to migrate the logic.

We will see.

If it does work... that's our v0.4.0 build, as well as v0.31.0 of TNT, both of which will need to be published (in the opposite of the order just listed).

thombruce commented 4 weeks ago

That's not worked. Path looks the same:

<a href="#/C:\Users\thom\Documents\Notes\todo.txt" class="">todo.txt</a>

PORTABLE dir should be C:\Users\thom\Documents\Notes\ which should result in just todo.txt after the replace operation.

Maybe PORTABLE var is normalized then... or maybe it's not a match; maybe that dir is more complex.

Another option, perhaps a better option... is to obtain the root directory's path (since we get this with the tree anyway) and remove that from all children and descendants.

This path is guaranteed to match the normalization, guaranteed to be similar to the descendants and guaranteed to be available to the renderer.

That should work.

thombruce commented 4 weeks ago

Resolved by TNT PR: https://github.com/thombruce/tnt/pull/87

This made its way into v0.31.0 of TNT and TNT Electron, which we've bumped to here as of v0.4.0-alpha.2 followed by the non-alpha release of v0.4.0.