Notification Messages: prepareMessages queue bug

[cleidigh]

Insiders 9-13-2017
Windows 7

While working on adding keyboard navigation (#32034) for global message notifications I ran into a tracking problem regarding how messages are queued and aggregated (eliminate common messages by text).
The problem is prepareMessages goes through the message queue from newest to oldest
messages. If a new message is added that matches an older message, it can take a slot
that is not rendered but, will bump the older message from the rendered list.

The fix is to go backwards in the global list and make sure the older messages are rendered
first.

I would like to do this PR before I do the full keyboard navigation PR.

Steps to reproduce (requires test code):

• Create a TestMessages command

  • Generates 6 test action messages (eliminate purged messages for simplicity)
  • Each of the six messages our unique eg Msg1 ... Msg6

• Create a TestMessages2 command

  • Generates 1 test action message Msg1 (creates duplicate first message above)

• Execute TestMessages

  • Results in the following message is being displayed:
    Msg5
    Msg4
    Msg3
    Msg2
    Msg1
  • Msg6 is added to the queue but not shown (correct behavior so far)

• Execute TestMessages2

  • Results in the following message is being displayed:
    Msg6
    Msg5
    Msg4
    Msg3
    Msg2
  • The new Msg1 gets processed by prepareMessages and added to handledMessages
  • The oldest Msg1 is that a duplicate and not rendered, but the five messages
  • rendered are the first five messages without the duplicate
  • Unless I am misunderstanding the design, I believe the following should be correct
    Msg5
    Msg4
    Msg3
    Msg2
    Msg1
  • Msg6 still hidden as well as the duplicate (but newer) Msg1

The proposed/tested fix:

• scan the global message list backwards

• build the rendered message list forward (oldest first)

• reverse the list (now with oldest last)

• Splice message array to only include maxMessages

  private prepareMessages(): IMessageEntry[] {

    // Aggregate Messages by text to reduce their count
    const messages: IMessageEntry[] = [];
    const handledMessages: { [message: string]: number; } = {};
  
    let offset = 0;
  
    // Process messages from oldest to newest
    // Fixes dropping duplicate message
  
    for (let i = this.messages.length - 1; i >= 0; i--) {
    	const message = this.messages[i];
    	if (types.isUndefinedOrNull(handledMessages[message.text])) {
    		message.count = 1;
    		messages.push(message);
    		handledMessages[message.text] = offset++;
    	} else {
    		messages[handledMessages[message.text]].count++;
    		console.debug(messages[handledMessages[message.text]].count);
    	}
    }
  
    // Reverse messages to go from newest to oldest rendering
    messages.reverse();
  
    if (messages.length > this.options.maxMessages) {
    	return messages.splice(messages.length - this.options.maxMessages, messages.length);
    }
  
    return messages;
  

  }

[bpasero]

Please note that the message widget is considered deprecated and I suggest to stay away from touching it until we have a new system in place. Changing something there would be waste because we want to revisit notification system entirely (via #22388) which likely would result in a rewrite.

[bpasero]

The fix is to go backwards in the global list and make sure the older messages are rendered
first.

I am not sure that is correct though, because a newer message should overwrite an existing one (it could have other actions).

Anyhow, as I said before, this code is considered to be removed soon when we work on a new notification story...

[bpasero]

This landed via #43770 and can be tried out in our preview release: https://code.visualstudio.com/insiders/

[bpasero]

@cleidigh maybe you verify this works with the new implementation

[cleidigh]

@bpasero
very excited to try out
have to figure out the best way to generate errors
probably best if I put together a bit of test code
👍

[bpasero]

@cleidigh yeah or you write an extension that uses the API for showing messages and run that.

[joaomoreno]

@bpasero for steps

[bpasero]

@joaomoreno see #34422 (comment)

@cleidigh did you try it out?

[cleidigh]

@bpasero
The new notification framework is great !
I'm working on testing a variety of things now in particular keyboard stuff.
I have to look at the differences for this issue.
I've already found a couple things and posted new issues for those.

Go ahead and feel free to assign relevant issues for me to fix...

[bpasero]

@cleidigh thanks, happy for feedback