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

Fix checkbox widget when listIndex field undefined #7679

Merged

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Aug 15, 2023

When listIndex was pointing to an entry that didn't exist yet, so that extractTiddlerDataItem returned undefined, the checkbox widget was aborting and refusing to proceed. This is not the desired behavior: we want it to create the index if it didn't exist yet. So now we explicitly check for undefined and treat it the same as an empty list.

Also fixed one code-style issue in the same file while I was in here.

Fixes #7678.

When listIndex was pointing to an entry that didn't exist yet, so that
extractTiddlerDataItem returned undefined, the checkbox widget was
aborting and refusing to proceed. This is not the desired behavior: we
want it to create the index if it didn't exist yet. So now we explicitly
check for undefined and treat it the same as an empty list.

Also fixed one code-style issue in the same file while I was in here.
@vercel
Copy link

vercel bot commented Aug 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
tiddlywiki5 ✅ Ready (Inspect) Visit Preview Aug 15, 2023 4:12pm

@rmunn
Copy link
Contributor Author

rmunn commented Aug 15, 2023

An alternative way to fix this bug would have been to change

https://github.com/Jermolene/TiddlyWiki5/blob/641d92ef7b344718a5eb1c6d76722846fc939c98/core/modules/widgets/checkbox.js#L213

to be:

 fieldContents = this.wiki.extractTiddlerDataItem(this.checkboxTitle,this.checkboxListIndex,[]);

@Jermolene, please let me know if you would prefer this one-line fix instead of the two-line fix I ultimately went with, and I'll change it. I decided in favor of the two-line fix because it would let me create a later PR that cleans up a bit of the code and makes one use case slightly (very slightly) more performant. (If you're wondering, it would let me stop making copies of an empty array, leading to one less small object that the browser needs to garbage-collect. A very, very small performance win, and not worth the added complexity in a PR just days before release; I'll get that PR into 5.3.2).

@rmunn
Copy link
Contributor Author

rmunn commented Aug 15, 2023

@Jermolene This is a bugfix for a bug I introduced in 5.3.0, so I'd love to get it into 5.3.1 if possible. I know it's post-freeze, but since this is such an easy-to-review bugfix for a rather annoying bug (the listIndex feature basically doesn't work without this bugfix), I'd like to request a freeze exception for this PR, and ask you to look at it and merge it in before 5.3.1.

@Jermolene
Copy link
Member

Great thanks @rmunn I think that this is worth including in v5.3.1.

rmunn added a commit to rmunn/TiddlyWiki5 that referenced this pull request Aug 17, 2023
@rmunn rmunn deleted the bugfix/checkbox-listindex-undefined branch August 17, 2023 02:44
Jermolene pushed a commit that referenced this pull request Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] CheckboxWidget listIndex mode no longer working after date field fix
3 participants