messagegui: refactor the messages scroller and use it more (WIP)

[thyttan]

WIP

Initializes the scroller with three all messages loaded, scrolled to the chosen message. You can scroll up/down to see the rest of the messages.

This is far from merge ready and I'm not sure it should ever be merged. Should be implemented as a setting if to be merged I think. I'm inching closer to something that could be considered for merging.

With that said it can be tried from my app loader: https://thyttan.github.io/BangleApps/?id=messagegui

[thyttan]

I'm testing/developing this by generating test messages from the WebIDE console field by pasting:

let i = 0;
let msgInterval = setInterval(
  ()=>{  
    print(i);
    GB({"t":"notify","id":1575479849+i,"src":"Hangouts","sender":"sender"+i,"title":"A Name"+i,"body":"message contents that are very similar to one another. in that they are somewhat long, and exactly the same. Let's have it go on and on and so forth for a while to make sure we overflow properly."});
    i++;
    if (i == 9) {clearInterval(msgInterval); delete i; delete msgInterval;}
  }, 500
);

[thyttan]

@bobrippling If you'd help me by testing this a bit I'd be interested to hear your thoughts!

[bobrippling]

@bobrippling If you'd help me by testing this a bit I'd be interested to hear your thoughts!

Sorry I missed this the first time round - taken a look, I like it, very nifty feature to have! And it's quite easily discoverable too. Means I have less cause to dig my phone out, works really well.

I do have two thoughts:

• The scrolling direction makes sense on the message overview but then on returning to the router the direction is then the opposite. Maybe we swap that around, or add an option for it? (happy to leave this to another PR)
• When scrolling down there's a bit of a yank/lag as it loads in the next message, but oddly not if I go up and load in the previous. Do you see that?

[thyttan]

very nifty feature to have! And it's quite easily discoverable too. Means I have less cause to dig my phone out, works really well.

Thanks! I wanted the bigger font of the scroller for what pops up for legibility's sake.

I do have two thoughts:

• The scrolling direction makes sense on the message overview but then on returning to the router the direction is then the opposite. Maybe we swap that around, or add an option for it? (happy to leave this to another PR)

Yeah, this feels a bit unfortunate to me as well.

• For the scroller it feels right to me how I've made it so the newest message is the one furthest to the top (similar to the list menu screen).
• For the overview screen (the function renamed from just "showMessage" to "showMessageOverview") that comes to the discussion of what is up/and down motions w r t swipes. People have different interpretations based on previous experiences as discussed here. When implementing the swipes I went with the style I believe Gordon uses for swipe interactions.
  • My thinking is that a relatively good solution to the different intuitions people have regarding this is to develop the Swipe inversion app more (initial PR: swipeinv: new app #3217).

• When scrolling down there's a bit of a yank/lag as it loads in the next message, but oddly not if I go up and load in the previous. Do you see that?

Yes, that is partly a limitation of the implementation. But I also find it a bit convenient that the scrolling stops at the end (or start, depending of if you scroll up or down) of the current message. So I don't scroll past the message by accident (I use kinetic scroll for my scrollers). The yank happens when another message is added to the array of wrapped strings and the scroller is reinited with that updated array. The yank (currently actually a 40ms timeout) also makes sure there are not other graphical glitches while loading in the next/previous message.

The yank doesn't happen when you go back because those messages are already part of the array of wrapped strings.

On another note, I'm a bit unhappy with how involved the code for processing, passing around and updating the alreadyProcessed data in showMessagesScroller is now. It feels like it's not super easy to read at first glance?

[bobrippling]

Thanks! I wanted the bigger font of the scroller for what pops up for legibility's sake.

Nice idea, I've not been using my watch with GB for a little while but this might bring it back. And your podcast controls I've recently spotted too - in that other thread

• The scrolling direction makes sense on the message overview but then on returning to the router the direction is then the opposite.
  Yeah, this feels a bit unfortunate to me as well.

• For the scroller it feels right to me how I've made it so the newest message is the one furthest to the top (similar to the list menu screen).
• For the overview screen (the function renamed from just "showMessage" to "showMessageOverview") that comes to the discussion of what is up/and down motions w r t swipes. People have different interpretations based on previous experiences as discussed here. When implementing the swipes I went with the style I believe Gordon uses for swipe interactions.

Yeah that's reasonable, I hadn't actually clocked the order of the messages, I think that works well. I suppose we could always try it for a while, perhaps invert it ourselves and see if that feels more "natural". Or yes - try with swipeinv

• When scrolling down there's a bit of a yank/lag as it loads in the next message, but oddly not if I go up and load in the previous. Do you see that?
  Yes, that is partly a limitation of the implementation. But I also find it a bit convenient that the scrolling stops at the end (or start, depending of if you scroll up or down) of the current message. So I don't scroll past the message by accident (I use kinetic scroll for my scrollers). The yank happens when another message is added to the array of wrapped strings and the scroller is reinited with that updated array. The yank (currently actually a 40ms timeout) also makes sure there are not other graphical glitches while loading in the next/previous message.

That's a good point about halting in fact - I think with kineticscroll especially it'll stand out as a clear and intentional stop. I think like above, we could try this for a while and see how it feels

On another note, I'm a bit unhappy with how involved the code for processing, passing around and updating the alreadyProcessed data in showMessagesScroller is now. It feels like it's not super easy to read at first glance?

Yes I haven't fully got my head round it - I jumped in with a live test of it instead! I can have a go at suggesting some alterations (if I can come up with any), if you would find it useful to have another point of view?

[thyttan]

That's a good point about halting in fact - I think with kineticscroll especially it'll stand out as a clear and intentional stop. I think like above, we could try this for a while and see how it feels

On another note, I'm a bit unhappy with how involved the code for processing, passing around and updating the alreadyProcessed data in showMessagesScroller is now. It feels like it's not super easy to read at first glance?

Yes I haven't fully got my head round it - I jumped in with a live test of it instead! I can have a go at suggesting some alterations (if I can come up with any), if you would find it useful to have another point of view?

I've refactored the scroller to just load in all the messages from the start. Makes the logic much simpler. The halting is gone by that token, not sure if it's a plus or minus.

But that meant I had to implement some way of deciding what messages should be deemed read by the user (i.e. msg.new=false). If there was a better way to do that I'd be happy - but I can't think of one now.

I'm now at a point where I don't think I'll go much further with the code by myself. I'd be thankful for you to give it a review @bobrippling! But no rush if you'd want to live test it some first, again.

[bobrippling]

Not forgot - will get round to this soon :)

[bobrippling]

I've refactored the scroller to just load in all the messages from the start. Makes the logic much simpler. The halting is gone by that token, not sure if it's a plus or minus.

Nice, yeah this works very well. I do see the messages in a different order now (after running your setup code) - zoomed out to the summary view:

Is this expected?

But that meant I had to implement some way of deciding what messages should be deemed read by the user (i.e. msg.new=false). If there was a better way to do that I'd be happy - but I can't think of one now.

No I think this works for now - just to check, we're not off by one anywhere are we? Maybe we go up to just shownMsgIdxLast? And the user has to go to the end of it to mark it as read?

BangleApps/apps/messagegui/app.js

Lines 396 to 398 in 447f150

	for (let i=shownMsgIdxFirst; i<shownMsgIdxLast+1; i++) {
	MESSAGES[i].new = false;
	}

I'm now at a point where I don't think I'll go much further with the code by myself. I'd be thankful for you to give it a review @bobrippling! But no rush if you'd want to live test it some first, again.

Thanks - live tested and it looks good to me, just the two questions I have above :)

[thyttan]

I've refactored the scroller to just load in all the messages from the start. Makes the logic much simpler. The halting is gone by that token, not sure if it's a plus or minus.

Nice, yeah this works very well. I do see the messages in a different order now (after running your setup code) - zoomed out to the summary view:

Is this expected?

I never understood why they ended up not entirely in order from 0 up to 8 (or maybe the other way around) before either. Or why the order changed with this latest change. Something about code execution time in different parts of the app? Maybe there's some logic somewhere that is a little off. Or not. But I think it is probably not a problem in normal use...

But that meant I had to implement some way of deciding what messages should be deemed read by the user (i.e. msg.new=false). If there was a better way to do that I'd be happy - but I can't think of one now.

No I think this works for now - just to check, we're not off by one anywhere are we? Maybe we go up to just shownMsgIdxLast? And the user has to go to the end of it to mark it as read?

BangleApps/apps/messagegui/app.js

Lines 396 to 398 in 447f150

	for (let i=shownMsgIdxFirst; i<shownMsgIdxLast+1; i++) {
	MESSAGES[i].new = false;
	}

Hm. I looked at the logic again and I think it's not wrong. But maybe it should always mark the message we open to as read even if the user didn't scroll down to see all of that message's contents?

What is it that makes you suspect an off by one miss? So I know what I'm looking for. EDIT: Did a change in 1c19b13 possibly fixing an off by one.

[bobrippling]

I never understood why they ended up not entirely in order from 0 up to 8 (or maybe the other way around) before either. Or why the order changed with this latest change. Something about code execution time in different parts of the app? Maybe there's some logic somewhere that is a little off. Or not. But I think it is probably not a problem in normal use...

Strange - perhaps a timing issue with the GB() callback and scheduling. Yes I don't think it's a problem though

Hm. I looked at the logic again and I think it's not wrong. But maybe it should always mark the message we open to as read even if the user didn't scroll down to see all of that message's contents?

What is it that makes you suspect an off by one miss? So I know what I'm looking for. EDIT: Did a change in 1c19b13 possibly fixing an off by one.

No you're right, I double checked and all is good - I was thinking it might trigger too early as the user scrolls not a full page down but you'd got it right already :)

[thyttan]

Tried the updates here on Bangle.js 1 emulator. I found two problems so far:

1. Widget Utils seems to have become incompatible with Bangle.js 1 at some point because of it's reliance on Bangle.setLCDOverlay.

>eval(require("Storage").read("messagegui.app.js"))
=undefined
Uncaught Error: Function "setLCDOverlay" not found!
 at line 16 col 549 in messagegui.app.js
exports.offset=-24,Bangle.setLCDOverlay(undefined,{id:'widge...
                          ^
in function "cleanupOverlay" called from line 16 col 863 in messagegui.app.js
exports.cleanupOverlay(),delete exports.offset,exports.swipe...
                       ^
in function "cleanup" called from line 16 col 105 in messagegui.app.js
if(exports.cleanup(),!global.WIDGETS)return;g.reset();for(va...
                   ^
in function "hide" called from line 308 col 11 in messagegui.app.js
  WU.hide();
          ^
in function "showMessagesScroller" called from line 640 col 40 in messagegui.app.js
    showMessagesScroller(newMessages[0]);
                                       ^
in function "checkMessages" called from line 745 col 22 in messagegui.app.js
    dontStopBuzz: 1 });
                     ^
in function called from system

Note: [Bangle.setLCDOverlay] is only available in Bangle.js 2 smartwatches and DICKENS

2. The scroller works with my test messages, but when I try to exit to the message overview I get low memory error:

>
Execution Interrupted during event processing.
New interpreter error: LOW_MEMORY,MEMORY
Execution Interrupted during event processing.
Execution Interrupted during event processing.
Execution Interrupted during event processing.
Execution Interrupted during event processing.
Execution Interrupted during event processing.

[thyttan]

... Sorry for force pushing again. I'll try to get into the habit of not rebasing and merge in upstream/master instead...

The earliest new commit this time is messagegui: add back persist variable that got lost f01b30d.

[bobrippling]

Ah no worries, that works perfectly well if you're able to remember the earliest new commits :) Shall take a look through if you're happy with the latest changes?

[thyttan]

Only look through if you feel like it - I'm not particularly looking for more feedback right now. Still have some stuff to work through.