Hi @iocanel

Sounds like a great idea!

I tried it out, but couldn't really get it to work as expected. Maybe I'm using it wrong. I tried adding a new line a table.

Given this header with a table (taken from sample.org)

* Tables
Try playing around with this one by first clicking on the table:

| Dog name | Age | Weight (in lbs) | Parent   | Score (1-10) |
|----------+-----+-----------------+----------+--------------|
| Eloise   |   3 |             5.1 | Erin     |           15 |
|----------+-----+-----------------+----------+--------------|

and this capture template:

Then, when I want to add something to the table for the first time, I get this result:

* Tables
Try playing around with this one by first clicking on the table:

| Dog name | Age | Weight (in lbs) | Parent   | Score (1-10) |
|----------+-----+-----------------+----------+--------------|
| Eloise   |   3 |             5.1 | Erin     |           15 |
|----------+-----+-----------------+----------+--------------|

| <2023-01-15 Sun> | [2023-01-15 Sun] | 2023-01-15 Sun | Jordan | 15 |

There's one newline too many, so it's two tables now.

When I call the capture template one more time (a second time), this is the result:

* Tables
Try playing around with this one by first clicking on the table:

| Dog name | Age | Weight (in lbs) | Parent   | Score (1-10) |
|----------+-----+-----------------+----------+--------------|
| Eloise   |   3 |             5.1 | Erin     |           15 |
|----------+-----+-----------------+----------+--------------|

| <2023-01-15 Sun>   | [2023-01-15 Sun] |              2023-01-15 Sun | Jordan   |           15 || <2023-01-15 Sun>   | [2023-01-15 Sun] |              2023-01-15 Sun | Jordan   |           15 |

So the table row from the second call to the capture template is added to the right of the row from the first call to the capture template.

In summary, both the first and the second call to the capture template did not append to a table. Here's how it looks in the GUI to me:

Another issue is that this new setting will override the behavior of previous capture templates. Here's a capture template that I use on a daily basis:

Apart from these two findings, I like the general idea! I'm not a hundred on whether or now I want to see the two boolean flags every time I use a capture templates, but maybe it's a good thing. I'll sleep on it(;

Let me change the default value, so that existing templates don't break and see what I can do with the newline handling.

I updated the PR with two new commits that handle spaces and the absence of shouldCaptureAsNewHeader.

Hi @iocanel

Thank you for taking another look at this! I just tested again by pulling your branch and restarting the server. Unfortunately, existing capture templates aren't treated as 'capture as new header'. I made a video to demonstrate: https://www.dropbox.com/s/s120b3q8vu19dhd/2023-01-28%2011-35-56.mkv?dl=0

Unfortunately, this would break existing capture templates, so i cannot merge at this time.

@munen: Thanks for the detailed feedback. Let me have a look.

Apart from that, I'd like to discuss your choice to add "Should capture as new header" to the <CaptureModal> component. I know, we had the 'Prepend?' question in there as well, even though it is configurable in the capture template itself. For prepending, I can think of scenarios where I'd like to change my mind on the fly - for example usually I add todos to the bottom of the inbox, but every once in a while, when it's super important, I might change my mind to add it to the top. In truth, I personally never do that, but I can imagine such a person(;

When it comes to 'should capture as new header', I cannot really think of such a template. If it's a "TODO" header, it's always a new header. If it's inline content, like a new table row, it never is. Did you have a scenario in mind where you need to switch the flag on the fly?

Wow, just 5 minutes response time, even though it took me a while to get back to your changes. You're a 🚀, @iocanel 💯 (;

Wow, just 5 minutes response time, even though it took me a while to get back to your changes. You're a rocket, @iocanel 100 (;

I just happened to be around. 😄

BTW, I am not able to reproduce what I am seeing on the video, which makes me wonder if the template was tainted (as if persisted with captureAsNewHeader false) by the original pull request.

Give me a sec, to share what I experience myself.

Ok, it seems that I was the one with the tainted templates after 😊

which makes me wonder if the template was tainted (as if persisted with captureAsNewHeader false) by the original pull request.

When I tested, I thought the same. So I checked my .organice-config.js file and didn't see a captureAsNewHeader flag. My apologies for not mentioning it before.

Ok, it seems that I was the one with the tainted templates after blush

hihi, it happens to the best 😄

Pushed a fix. Just to be sure I switched to the master branch and recreated my templates from scratch. So, hopefully it should work fine.

Regarding the model window, if felt that it rhymes with the prepend, but I don't have a use case forcing it. So, I don't mind removing it.

@iocanel Sweet! When using my old capture templates, they do show 'create new header', now!

However, your fix is 'only' inline the capture template modal. Other than patching new functionality inline, I think a more solid approach is to write a migration (https://github.com/200ok-ch/organice/tree/master/src/migrations), so that all existing capture templates will actually have the new flag set to true. Otherwise all places would have to be patched inline which makes for hard to reason about code. Here is one more (likely the last) spot: While existing capture templates do work, they show the wrong configuration in the editor:

Also, I've tested adding things to header descriptions like tables and it works a treat. I did find a bug concerning tables, but we can totally leave it if you think it's an edge case: https://www.dropbox.com/s/cnjrlqq37ib506m/2023-01-28%2013-01-44.mkv?dl=0

Regarding the model window, if felt that it rhymes with the prepend, but I don't have a use case forcing it. So, I don't mind removing it.

If you only added it for consistency, but also don't see a personal reason to use it, I'd say thank you for taking the extra effort to make it consistent, but let's remove the feature that we both see no reason to use(;

When these things are done, the last part would be to get the tests to be green, again.

Ok, I'll need to check how migrations are used.

I am tying something like:

import { localStorageAvailable } from '../util/settings_persister';

export default () => {
  if (!localStorageAvailable) {
    return;
  }

  let captureTemplates = JSON.parse(localStorage.getItem('captureTemplates'));
  captureTemplates = captureTemplates || {};

  captureTemplates.forEach(t => {
    if (!t.hasOwnProperty('shouldCaptureAsNewHeader')) {
      t.shouldCaptureAsNewHeader = true
    }
  });
  localStorage.setItem('captureTemplates', JSON.stringify(captureTemplates));
};

Even though localStorage seems to get updated correctly the updated templates do not seem to get persisted.

I updated the PR, but it's not workign as expected. Will have another look later. Meanwhile if there is soemthing obviously missing, please let me know.