espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.76k stars 741 forks source link

Bangle 1: E.showScroller() triangle drawn over top item #2153

Closed andrewgoz closed 2 years ago

andrewgoz commented 2 years ago

Using the sample E.showscroller code from the documentation, when run on the Bangle.js 1 emulator the top triangle is drawn over the list items.

When I originally posted about this, Gordon suggested it was intended to work with widgets. It does look better with widgets enabled, but that would mean the triangle overdraws the widgets instead?

gfwilliams commented 2 years ago

but that would mean the triangle overdraws the widgets instead?

Only if there are widgets right in the middle, which is reasonably rare since they grow out from either side.

If you have suggestions for where to put the up arrow that'd be better I'm open to ideas :)

andrewgoz commented 2 years ago

Are they needed? I just compared a similar UI on the Pebble, which has a similar three-button hardware UI to the Bangle1.

The way they handle a scroller is that they always try to keep the currently selected item in the center of the screen, with the other items above and below it. It's only when the selected item approaches either the top or bottom of the list that the drawing logic forces the highlight up or down as required to highlight the correct item, but also not allowing the top item to drop down and reveal empty space (and same for the bottom item - it doesn't shift up to the middle - the highlight moves down to the bottom of the screen).

That way triangles aren't needed at all.

andrewgoz commented 2 years ago

Here is a demo you can copy-paste into the IDE. I've re-written the Bangle 1 (non-Q3) scroller to work in a Pebble-like fashion.

Edit: changed to pass selection flag to draw function, and make selection box drawing optional - still backward compatible with existing code

var scroller;

var testScroller = (function(options) {
  /* options = {
    h = height
    c = # of items
    draw = function(idx, rect)
    select = function(idx)
  }*/

if (!options) return Bangle.setUI();
var selected = 0;
var w = Bangle.appRect.w;
var h = Bangle.appRect.h;
var X = Bangle.appRect.x;
var Y = Bangle.appRect.y;

function drawScroller(idx) {
  g.reset();
  // prefer drawing the list so that the selected item is in the middle of the screen
  var y=((h-options.h)/2)-selected*options.h;
  var by=y+options.c*options.h;
  if (by<=h) y += (h-by);
  if (y>0) y = 0;
  // draw
  for (var i=0;i<options.c;i++) {
    if ((idx===undefined)||(idx===i)) {
      if ((y>-options.h+1)&&(y<h)) {
        var y1 = Math.max(Y,Y+y);
        var y2 = Math.min(Y+h-1,Y+y+options.h-1);
        g.setColor((i==selected)?g.theme.fgH:g.theme.fg)
         .setBgColor((i==selected)?g.theme.bgH:g.theme.bg)
         .setClipRect(X,y1,X+w-1,y2);
        if (!options.draw(i,{x:X,y:Y+y,w:w,h:options.h},i==selected)) {
          // border for selected
          if (i==selected) {
            g.setColor(g.theme.fgH)
             .drawRect(X,Y+y,w-1,Y+y+options.h-1)
             .drawRect(X+1,Y+y+1,w-2,Y+y+options.h-2);
          }
        }
      }
    }
    y+=options.h;
  }
}
g.reset().clearRect(X,Y,w-1,h-1);
drawScroller();
Bangle.setUI("updown",dir=>{
  if (dir) {
    selected += dir;
    if (selected<0) selected = options.c-1;
    if (selected>=options.c) selected = 0;
    drawScroller();
  } else {
    options.select(selected);
  }
});
return {
  draw : drawScroller,
  drawItem : drawScroller
};
});

function show() {
  scroller = testScroller({
    h: 27,
    c: 20,
    draw: (idx, r) => {
      g.clearRect(r.x, r.y, r.x + r.w - 1, r.y + r.h - 1)
       .setFontAlign(0, 0)
       .setFont("6x8:2")
       .drawString((selected?"*":"")"Item "+idx, r.x + r.w / 2, r.y + r.h / 2);
      return true;
    },
    select: i => print("You selected "+i)
  });
}
Bangle.loadWidgets();
Bangle.drawWidgets();
show();
gfwilliams commented 2 years ago

Thanks - but I see this:

screenshot

So I'm not sure how it's apparent to the user that there are any items available after 'Item 8'?

IIRC we never originally had up/down arrows for menus, but they were added at the request of users - so I'm not convinced that just removing them is suddenly going to be fine with those people that complained previously.

You could always submit the change as an app and see how folks feel about it though: https://www.espruino.com/Bangle.js+Modification

gfwilliams commented 2 years ago

Just to add, maybe we can have the best of both worlds:

What if the top item (Item 0) stays as Item 0 until the page scrolls, and then it turns into an up-arrow. Same with the bottom-most item

andrewgoz commented 2 years ago

It isn't immediately apparent that there is more than Item 8, that is true, but as soon as the user starts navigating down it will become apparent there are more options, then when they get to the end it is also obvious that they have reached the end.

All I can say is that over the years I've been with the Pebble, I haven't found the lack of indication a problem, nor have I heard of such complaints.

I'll have a fiddle with adding the triangles back...

In the mean time, I've just edited my example code above to allow the draw function to know if it's drawing the selected item, and also to optionally inhibit drawing of the selection rectangle. That's mostly to match the Pebble appearance. The changes don't affect existing code conventions.

andrewgoz commented 2 years ago

Triangles, as requested :-) (edit: close drawPoly()s, edit2: add startidx, edit3: startidx->scroll)

var scroller;

var testScroller = (function(options) {
  /* options = {
    h = height
    c = # of items
    draw = function(idx, rect)
    select = function(idx)
  }*/

if (!options) return Bangle.setUI();
var selected = 0;
if (options.scroll) selected=options.scroll;
var w = Bangle.appRect.w;
var h = Bangle.appRect.h;
var X = Bangle.appRect.x;
var Y = Bangle.appRect.y;

function drawScroller(idx) {
  g.reset();
  // prefer drawing the list so that the selected item is in the middle of the screen
  var ty=((h-options.h)/2)-selected*options.h;
  var y=ty;
  var by=y+options.c*options.h;
  if (by<=h) y += (h-by);
  if (y>0) y = 0;
  // draw
  for (var i=0;i<options.c;i++) {
    if ((idx===undefined)||(idx===i)) {
      if ((y>-options.h+1)&&(y<h)) {
        var y1 = Math.max(Y,Y+y);
        var y2 = Math.min(Y+h-1,Y+y+options.h-1);
        g.setColor((i==selected)?g.theme.fgH:g.theme.fg)
         .setBgColor((i==selected)?g.theme.bgH:g.theme.bg)
         .setClipRect(X,y1,X+w-1,y2);
        if (!options.draw(i,{x:X,y:Y+y,w:w,h:options.h},i==selected)) {
          // border for selected
          if (i==selected) {
            g.setColor(g.theme.fgH)
             .drawRect(X,Y+y,w-1,Y+y+options.h-1)
             .drawRect(1,Y+y+1,w-2,Y+y+options.h-2);
          }
        }
      }
    }
    y+=options.h;
  }
  // arrows
  g.setClipRect(X,Y,X+w-1,Y+h-1);
  var m=w/2;
  var pt=[X+m,Y,X+m-14,Y+14,X+m+14,Y+14];
  var pb=[X+m,Y+h,X+m-14,Y+h-14,X+m+14,Y+h-14];
  if (ty<0) {
    g.setColor(g.theme.fg)
     .fillPoly(pt)
     .setColor(g.theme.bg)
     .drawPoly(pt,true);
  }
  if (by>h) {
    g.setColor(g.theme.fg)
     .fillPoly(pb)
     .setColor(g.theme.bg)
     .drawPoly(pb,true);
  }
}
g.reset().clearRect(X,Y,w-1,h-1);
drawScroller();
Bangle.setUI("updown",dir=>{
  if (dir) {
    selected += dir;
    if (selected<0) selected = options.c-1;
    if (selected>=options.c) selected = 0;
    drawScroller();
  } else {
    options.select(selected);
  }
});
return {
  scroll : () => selected,
  draw : drawScroller,
  drawItem : drawScroller
};
});

function show() {
  scroller = testScroller({
    h: 27,
    c: 20,
    draw: (idx, r, selected) => {
      g.clearRect(r.x, r.y, r.x + r.w - 1, r.y + r.h - 1)
       .setFontAlign(0, 0)
       .setFont("6x8:2")
       .drawString((selected?"*":"")+"Item "+idx, r.x + r.w / 2, r.y + r.h / 2);
      return true;
    },
    select: i => print("You selected "+i)
  });
}
Bangle.loadWidgets();
Bangle.drawWidgets();
show();
gfwilliams commented 2 years ago

Thanks! That looks good to me - it'd be nice to set and accept a scroll parameter as the current version does, so menus can choose to redraw at the same point (eg if coming from a submenu).

Otherwise a PR would be great :)

andrewgoz commented 2 years ago

I could never work out what scroll was supposed to do. I tried a local copy of the original scroller (with the options.scroll fix), but couldn't see it do anything. If it was meant to set the initial selection, then 'scroll' isn't a great name. I'm at work now so can't do much, but I'll edit my latest code above with an optional 'startidx' parameter.

gfwilliams commented 2 years ago

If it was meant to set the initial selection, then 'scroll' isn't a great name

The issue is really that we want it to be compatible with code that also runs on Bangle.js 2 where it really is the amount of pixels scrolled - so adding a separately named item isn't helpful. The idea is if some app copies the scroll value and puts it back in when calling E.showScroller again then it ends up with the same item selected. It doesn't really matter what's in that value.

andrewgoz commented 2 years ago

Thanks for explaining the scroll parameter. That also explains the use of the scroll function in the returned object in the Q3 version, which I note is missing in this version. I'll do a PR.