~whereswaldon/arbor-dev

3 2

[PATCH] Automatically split paragraphs

Details
Message ID
<20200922024757.GA10142@rex.lan>
DKIM signature
missing
Download raw message
Alright, let's see if this patch works.
Had to generate it manually, hoping it applies cleanly
Details
Message ID
<C5TWGOLSEUDT.1NQ1OL16GJ7KG@pop-os>
In-Reply-To
<20200922024757.GA10142@rex.lan> (view parent)
DKIM signature
pass
Download raw message
Hey Danny,

This patch only applies with `patch` (not `git apply`) because completely
empty lines in the diff have no leading space in the "gutter" to indicate
that there was no change on that line. Did you generate this with `diff`?

I've applied this and examined it for a while, and I have a new round of
questions/concerns. I'm really sorry that I didn't catch this stuff
previously (don't want to trap you in review purgatory), but I think some
of it is pretty important.

I'll paste the diff from your patch below so that I can comment on it.

Thanks for working on this!
Chris

> diff --git a/reply-view.go b/reply-view.go
> index 6a675d9..ea845fa 100644
> --- a/reply-view.go
> +++ b/reply-view.go
> @@ -304,14 +304,29 @@ func (c *ReplyListView) startReply() {
>  	c.ReplyEditor.Focus()
>  }
>  
> +func gatherParagraphs(text string) []string {
> +	var paragraphs []string
> +	stringIndex := 0
> +
> +	for _, paragraph := range strings.Split(text, "\n") {
> +		// Remember to chomp blank lines at the start of the message
> +		if paragraph == "" && len(paragraphs) > 0 {
> +			stringIndex += 1
> +		} else {
> +			if len(paragraphs) <= stringIndex {
> +				paragraphs = append(paragraphs, paragraph)
> +			} else {
> +				paragraphs[stringIndex] += ("\n" + paragraph)
> +			}
> +		}
> +	}
> +	return paragraphs
> +}

It hit me that this might be equivalent to strings.Split(text, "\n\n"). Do
we lose anything by doing it that way?

> +
>  func (c *ReplyListView) sendReply() {
> -	var newReply *forest.Reply
>  	var author *forest.Identity
>  	replyText := c.ReplyEditor.Text()
>  	replyText = strings.TrimSpace(replyText)
> -	if replyText == "" {
> -		return
> -	}
>  	nodeBuilder, err := c.Settings().Builder()
>  	if err != nil {
>  		log.Printf("failed acquiring node builder: %v", err)
> @@ -329,36 +344,52 @@ func (c *ReplyListView) sendReply() {
>  					}
>  				}
>  			})
> -			convo, err := nodeBuilder.NewReply(chosen, c.ReplyEditor.Text(), []byte{})
> -			if err != nil {
> -				log.Printf("failed creating new conversation: %v", err)
> -			} else {
> -				newReply = convo
> +			var parent *forest.Reply
> +			var convo *forest.Reply
> +			var err error
> +			for i, paragraph := range gatherParagraphs(replyText) {
> +				if i == 0 {
> +					convo, err = nodeBuilder.NewReply(chosen, paragraph, []byte{})
> +				} else {
> +					convo, err = nodeBuilder.NewReply(parent, paragraph, []byte{})
> +				}
> +
> +				if err != nil {
> +					log.Printf("failed creating new conversation: %v", err)
> +				} else {
> +					c.postReply(author, convo)
> +					parent = convo
> +				}
>  			}
>  		}
>  	} else {
> -		reply, err := nodeBuilder.NewReply(c.ReplyingTo.Reply, c.ReplyEditor.Text(), []byte{})
> -		if err != nil {
> -			log.Printf("failed building reply: %v", err)
> -		} else {
> -			newReply = reply
> -		}
> -	}
> -	if newReply != nil {
> -		go func() {
> -			if err := c.Arbor().Store().Add(author); err != nil {
> -				log.Printf("failed adding replying identity to store: %v", err)
> -				return
> -			}
> -			if err := c.Arbor().Store().Add(newReply); err != nil {
> -				log.Printf("failed adding reply to store: %v", err)
> -				return
> +		parent := c.ReplyingTo.Reply
> +
> +		for _, text := range gatherParagraphs(replyText) {
> +			convo, err := nodeBuilder.NewReply(parent, text, []byte{})
> +			if err != nil {
> +				log.Printf("failed building reply: %v", err)
> +			} else {
> +				c.postReply(author, convo)
> +				parent = convo
>  			}
> -		}()
> -		c.resetReplyState()
> +		}
>  	}
>  }
>  
> +func (c *ReplyListView) postReply(author *forest.Identity, reply *forest.Reply) {

We are now invoking this function within a loop, and that loop is producing nodes
that have a direct data dependency on one another. I think this introduces a race
condition in which a later loop's invocation of this function finishes executing
before an earlier one and the reply that it attempts to add is rejected because
its parent doesn't exist within the store yet.

To counter this, can we rewrite this function signature to be:

func (c *ReplyListView) postReplies(author *forest.Identity, replies ...*forest.Reply) {

We can then post the author followed by all of the replies (in the order in which
they are provided) without any races.

> +	go func() {
> +		if err := c.Arbor().Store().Add(author); err != nil {
> +			log.Printf("failed adding replying identity to store: %v", err)
> +			return
> +		}
> +		if err := c.Arbor().Store().Add(reply); err != nil {
> +			log.Printf("failed adding reply to store: %v", err)
> +			return
> +		}
> +	}()
> +	c.resetReplyState()
> +}
>  func (c *ReplyListView) processMessagePointerEvents(gtx C) {
>  	tryOpenLink := func(word string) {
>  		if u, err := url.ParseRequestURI(word); err == nil {
> 
Details
Message ID
<20200922135420.GA3397@duke.tekk.in>
In-Reply-To
<C5TWGOLSEUDT.1NQ1OL16GJ7KG@pop-os> (view parent)
DKIM signature
missing
Download raw message
On Tue, Sep 22, 2020 at 08:34:19AM -0400, Chris Waldon wrote:
> Hey Danny,
>
> This patch only applies with `patch` (not `git apply`) because completely
> empty lines in the diff have no leading space in the "gutter" to indicate
> that there was no change on that line. Did you generate this with `diff`?
No, it was generated with git diff. That was just to work around my
workflow being bad and not email friendly (which I'm planning on fixing.)

Basically I'm used to merging, which is all well and good at work but when
I'm sending patches via email it results in all of the merged patches
getting included in git-send-email, hence my need to use git diff for this
one.

> I've applied this and examined it for a while, and I have a new round of
> questions/concerns. I'm really sorry that I didn't catch this stuff
> previously (don't want to trap you in review purgatory), but I think some
> of it is pretty important.
I never care about review purgatory c:

>
> > diff --git a/reply-view.go b/reply-view.go
> > index 6a675d9..ea845fa 100644
> > --- a/reply-view.go
> > +++ b/reply-view.go
-- snip --
> > +	for _, paragraph := range strings.Split(text, "\n") {
> > +		// Remember to chomp blank lines at the start of the message
> > +		if paragraph == "" && len(paragraphs) > 0 {
> > +			stringIndex += 1
> > +		} else {
> > +			if len(paragraphs) <= stringIndex {
> > +				paragraphs = append(paragraphs, paragraph)
> > +			} else {
> > +				paragraphs[stringIndex] += ("\n" + paragraph)
> > +			}
> > +		}
> > +	}
> > +	return paragraphs
> > +}
>
> It hit me that this might be equivalent to strings.Split(text, "\n\n"). Do
> we lose anything by doing it that way?
My first reaction was that it'd mess up handling at the start of messages,
but as long as we filter out empty strings that should be fine? Will test
for sure and include in the next patch.

-- snip --
> > +func (c *ReplyListView) postReply(author *forest.Identity, reply *forest.Reply) {
>
> We are now invoking this function within a loop, and that loop is producing nodes
> that have a direct data dependency on one another. I think this introduces a race
> condition in which a later loop's invocation of this function finishes executing
> before an earlier one and the reply that it attempts to add is rejected because
> its parent doesn't exist within the store yet.
>
> To counter this, can we rewrite this function signature to be:
>
> func (c *ReplyListView) postReplies(author *forest.Identity, replies ...*forest.Reply) {
>
> We can then post the author followed by all of the replies (in the order in which
> they are provided) without any races.
>
I think you're right. The goroutines (as far as I know) aren't guaranteed to execute in
order or complete on their first execution. From practical experience testing it seems
unlikely but it could still totally happen. This'll be in the next round.
Details
Message ID
<CAFcc3FQEyttXbnh2ZT3pE=o4p_uW-3rp=bCe4WTqDjz-XbXDxQ@mail.gmail.com>
In-Reply-To
<20200922135420.GA3397@duke.tekk.in> (view parent)
DKIM signature
pass
Download raw message
Thanks! I appreciate your patience with the review process.
Reply to thread Export thread (mbox)