bradyt / taskw-dart

Taskwarrior-inspired mobile todo app
82 stars 8 forks source link

Task creation silently drops attribute-like use of colon, e.g. `foo:` or `foo:bar` #17

Closed danielquinn closed 2 years ago

danielquinn commented 2 years ago

In trying to create a few tasks last night, I discovered two oddities I thought worth reporting.

In the first case, I tried to create a task to write a new blog post. I called it Blog: Test and hit the little "enter" icon on the far right of the dialogue.

Creation with Colon

The resulting task however wasn't called Blog: Test, but just plain Test. I managed to reproduce this with a variety of different permutations, but strangely, X: Test came out as it should.

Creation result

Secondly, I sleepily tried to create an empty task.

Creation with nothing

That is, I opened the create task dialogue like before, but instead of typing anything, I just hit that "enter" icon again. The result was a hefty stack trace:

"\'" expected at 1:1

#0 Failure.value (package:petitparser/src/context/failure.dart:13)
#1 taskParser (package:taskw/src/task_parser.dart:60)
#2 _AddTaskBottomSheetState.submit (package:task/src/widgets/add_task_bottom_sheet.dart:34)
#3 _InkResponseState._handleTap (package:flutter/src/material/ink_well.dart:989)
#4 GestureRecognizer.invokeCallback (package:flutter/src/gestures/recognizer.dart:193)
#5 TapGestureRecognizer.handleTapUp (package:flutter/src/gestures/tap.dart:608)
#6 BaseTapGestureRecognizer._checkUp (package:flutter/src/gestures/tap.dart:296)
#7 BaseTapGestureRecognizer.acceptGesture (package:flutter/src/gestures/tap.dart:267)
#8 GestureArenaManager.sweep (package:flutter/src/gestures/arena.dart:157)
#9 GestureBinding.handleEvent (package:flutter/src/gestures/binding.dart:444)
#10 GestureBinding.dispatchEvent (package:flutter/src/gestures/binding.dart:420)
#11 RendererBinding.dispatchEvent (package:flutter/src/rendering/binding.dart:278)
#12 GestureBinding._handlePointerEventImmediately (package:flutter/src/gestures/binding.dart:374)
#13 GestureBinding.handlePointerEvent (package:flutter/src/gestures/binding.dart:338)
#14 GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:296)
#15 GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:279)
#16 _rootRunUnary (dart:async/zone.dart:1444)
#17 _CustomZone.runUnary (dart:async/zone.dart:1335)
#18 _CustomZone.runUnaryGuarded (dart:async/zone.dart:1244)
#19 _invoke1 (dart:ui/hooks.dart:185)
#20 PlatformDispatcher._dispatchPointerDataPacket (dart:ui/platform_dispatcher.dart:293)
#21 _dispatchPointerDataPacket (dart:ui/hooks.dart:98)

Neither of these are a big deal obviously and they're easy enough to work around, but I thought you'd wanna know about them :-)

bradyt commented 2 years ago

Thank you for the bug report.

I had a vague sense I'd made some things worse, as I allude to at https://github.com/bradyt/taskw-dart/issues/5#issuecomment-990143873, with:

Also I think anything like a:b will be silently dropped.

I appreciate you providing a usecase that gives me a better sense of how severe this is, and better motivation.

I've drafted a solution, where in my use of PetitParser, I will make an earlier check that the colon is being used in a known attribute, for example pri:H. This way, Blog: Test will be parsed as you expect, as description: 'Blog: Test'.

In that same comment I linked, I also mention I don't like my first attempt at quote parsing, and I plan to remove that feature. That would actually allow an empty description to pass through, so I think this is a good time to lift the field validation I have in other parts of the UI, to the Task constructor. Then, during task creation, it should show a more readable exception to the user, whether they enter an empty string, or an implicitly empty description like the following:

taskParser('+foo') // ==> Task(description: '', tags: ['foo'])

So I think I have a reasonable plan in place, some unit tests are already modified. I hope to soon work on testing these in the app and deploying.

EDIT: I misspoke. I think the UI code was already setup to have empty descriptions validated, as I think you'll see if you try to add a task simply as +foo. But lifting the validator from UI into the Task constructor should make it easier for developer to reason about, or test. The parser was erroring before the Task field validators were checked, and even if I remove the quote parsing, the parser still doesn't present a great error message in the case of the empty string, so I will probably add a special case for empty strings, so the Task constructor can be relied on for now with better error messages. In other words, taskParser will short circuit on the empty string, like taskParser('') ==> Task(description: ''), and the Task constructor will call validators which will throw an exception.

bradyt commented 2 years ago

The resulting task however wasn't called Blog: Test, but just plain Test. I managed to reproduce this with a variety of different permutations, but strangely, X: Test came out as it should.

The attribute parser is something like [A-Za-z]+:[A-Za-z]*, so I expect the current behavior silently drops strings like foo:bar, foo:, X:bar and X:. Can you clarify that you're seeing something like the following?

taskParser('X: test') // ==> Task(description: 'X: test')

I wasn't able to reproduce this. Maybe you were editing the description field of an already added task? There's no parsing in that case.

bradyt commented 2 years ago

I've started rolling out version 0.2.3, from v0.2.3/task/changelog:

  • Fix bugs in parsing of new task
    • Fix silent dropping of attribute-like terms containing colon, like foo:bar or foo:
    • Remove problematic quote parsing feature
    • Show FormatException if user submits empty string

I expect it to be available on iOS in about a day, and on Android in a few days. I suspect you use Android, so I can ping the thread when I am notified of Play Store, but F-Droid users should be able to rely on local notifications.

bradyt commented 2 years ago

Version 0.2.3 is available on both App Store and Play Store.

danielquinn commented 2 years ago

Sorry for taking so long to respond to these. I'm afraid I don't use GitHub that much (I'm more of a GitLab person). You'll be pleased to hear that the Blog: Test problem has been solved with your latest release though, so yay!

However, submitting an empty task still blows up a traceback, albeit a different one:

FormatException: Empty description will provoke a server error. (at character 1)

^
#0      validateTaskDescription (package:taskw/src/validate.dart:3)
#1      new Task._ (package:taskj/src/json/task.dart:40)
#2      new _$Task._ (package:taskj/src/json/task.g.dart:362)
#3      TaskBuilder.build (package:taskj/src/json/task.g.dart:624)
#4      new _$Task (package:taskj/src/json/task.g.dart:336)
#5      taskParser (package:taskw/src/task_parser.dart:76)
#6      _AddTaskBottomSheetState.submit (package:task/src/widgets/add_task_bottom_sheet.dart:34)
#7      _InkResponseState._handleTap (package:flutter/src/material/ink_well.dart:989)
#8      GestureRecognizer.invokeCallback (package:flutter/src/gestures/recognizer.dart:198)
#9      TapGestureRecognizer.handleTapUp (package:flutter/src/gestures/tap.dart:608)
#10     BaseTapGestureRecognizer._checkUp (package:flutter/src/gestures/tap.dart:296)
#11     BaseTapGestureRecognizer.acceptGesture (package:flutter/src/gestures/tap.dart:267)
#12     GestureArenaManager.sweep (package:flutter/src/gestures/arena.dart:157)
#13     GestureBinding.handleEvent (package:flutter/src/gestures/binding.dart:443)
#14     GestureBinding.dispatchEvent (package:flutter/src/gestures/binding.dart:419)
#15     RendererBinding.dispatchEvent (package:flutter/src/rendering/binding.dart:322)
#16     GestureBinding._handlePointerEventImmediately (package:flutter/src/gestures/binding.dart:374)
#17     GestureBinding.handlePointerEvent (package:flutter/src/gestures/binding.dart:338)
#18     GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:296)
#19     GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:279)
#20     _rootRunUnary (dart:async/zone.dart:1444)
#21     _CustomZone.runUnary (dart:async/zone.dart:1335)
#22     _CustomZone.runUnaryGuarded (dart:async/zone.dart:1244)
#23     _invoke1 (dart:ui/hooks.dart:169)
#24     PlatformDispatcher._dispatchPointerDataPacket (dart:ui/platform_dispatcher.dart:293)
#25     _dispatchPointerDataPacket (dart:ui/hooks.dart:88)
bradyt commented 2 years ago

Apologies! Rereading, I see you did emphasize the "hefty stack trace" aspect. I somehow read too quickly as I immediately did not like the unclear "\'" expected at 1:1 message when I added the task parsing. Personally, I find Empty description will provoke a server error. to be an improvement.

I would like to discourage filing an issue with multiple topics. Can you please file a new issue, and include in the new issue expected behavior? This would be a big favor to me. Especially as I find the severity and priority to vary between the two issues.

bradyt commented 2 years ago

I've retitled present issue thread and removed request of such from previous comment.

danielquinn commented 2 years ago

Sure no problem! Here you go: #18