Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Settings] Toggle checkbox partly obscured by widgets will draw over widgets. #2670

Closed
thyttan opened this issue Apr 1, 2023 · 7 comments
Closed
Labels
type-bug Something isn't working

Comments

@thyttan
Copy link
Collaborator

thyttan commented Apr 1, 2023

Affected hardware version

Bangle 2

Your firmware version

2v17.19

The bug

If there is a scroller active in a settings menu and I have scrolled down so that a checkbox is partly obscured by the widget field and I toggle the checkbox, the settings entry will draw over the widgets. The widgets will now be obscured until they are updated, either by their individual redraw logic or by a drawWidgets call.

The expectation would be for the checkbox to be toggled, but not draw over the widgets.

Reproduce:

  1. Upload the code pasted below to RAM with Web IDE
  2. On the watch scroll down so a checkbox is partly obscured by widgets
  3. toggle the obscured checkbox
  4. observe the menu entry is drawn over the widgets.

Based on updated WIP settings menu for Fastload Utils:

(function(back) {
  Bangle.loadWidgets();
  Bangle.drawWidgets();
  var FILE="fastload.json";
  var settings;
  var isQuicklaunchPresent = !!require('Storage').read("quicklaunch.app.js", 0, 1);

  function writeSettings(key, value) {
    var s = require('Storage').readJSON(FILE, true) || {};
    s[key] = value;
    require('Storage').writeJSON(FILE, s);
    readSettings();
  }

  function readSettings(){
    settings = require('Storage').readJSON(FILE, true) || {};
  }

  readSettings();

  function buildMainMenu(){
    var mainmenu = {};

    mainmenu[''] = { 'title': 'Fastload', back: back };

    mainmenu['Activate app history'] = {
        value: !!settings.useAppHistory,
        onchange: v => {
          writeSettings("useAppHistory",v);
          if (v && settings.autoloadLauncher) {
            writeSettings("autoloadLauncher",!v);  // Don't use app history and load to launcher together.
            setTimeout(()=>E.showMenu(buildMainMenu()), 0); // Update the menu so it can be seen if a value was automatically set to false (app history vs load launcher).
          }
        }
      };

    if (isQuicklaunchPresent) {
      mainmenu['Keep Quick Launch out of history'] = {
        value: !!settings.disregardQuicklaunch,
        onchange: v => {
          writeSettings("disregardQuicklaunch",v);
        }
      };
    }

    mainmenu['Force load to launcher'] = {
        value: !!settings.autoloadLauncher,
        onchange: v => {
          writeSettings("autoloadLauncher",v);
          if (v && settings.useAppHistory) {
            writeSettings("useAppHistory",!v);
            setTimeout(()=>E.showMenu(buildMainMenu()), 0); // Update the menu so it can be seen if a value was automatically set to false (app history vs load launcher).
          } // Don't use app history and load to launcher together.
        }
      };

    mainmenu['Hide "Fastloading..."'] = {
        value: !!settings.hideLoading,
        onchange: v => {
          writeSettings("hideLoading",v);
        }
      };

    mainmenu['This is here so the scroller is active'] = {
        value: !!settings.hideLoading,
        onchange: v => {
          writeSettings("hideLoading",v);
        }
      };

    return mainmenu;
  }

  E.showMenu(buildMainMenu());
})()

Installed apps

No response

@thyttan thyttan added the type-bug Something isn't working label Apr 1, 2023
@gfwilliams
Copy link
Member

Thanks - hmm, that's going to be a tricky one to fix. I guess if an item is clicked when it's partially off the screen we could scroll it until it was fully on?

@thyttan
Copy link
Collaborator Author

thyttan commented Apr 3, 2023

I tried adding setTimeout(Bangle.drawWidgets,0); to the onchange function and that solves the problem here. Like this:

(function(back) {
  Bangle.loadWidgets();
  Bangle.drawWidgets();
  var FILE="fastload.json";
  var settings;
  var isQuicklaunchPresent = !!require('Storage').read("quicklaunch.app.js", 0, 1);

  function writeSettings(key, value) {
    var s = require('Storage').readJSON(FILE, true) || {};
    s[key] = value;
    require('Storage').writeJSON(FILE, s);
    readSettings();
  }

  function readSettings(){
    settings = require('Storage').readJSON(FILE, true) || {};
  }

  readSettings();

  function buildMainMenu(){
    var mainmenu = {};

    mainmenu[''] = { 'title': 'Fastload', back: back };

    mainmenu['Activate app history'] = {
        value: !!settings.useAppHistory,
        onchange: v => {
          writeSettings("useAppHistory",v);
          if (v && settings.autoloadLauncher) {
            writeSettings("autoloadLauncher",!v);  // Don't use app history and load to launcher together.
            setTimeout(()=>E.showMenu(buildMainMenu()), 0); // Update the menu so it can be seen if a value was automatically set to false (app history vs load launcher).
          }
          setTimeout(Bangle.drawWidgets,0);
        }
      };

    if (isQuicklaunchPresent) {
      mainmenu['Keep Quick Launch out of history'] = {
        value: !!settings.disregardQuicklaunch,
        onchange: v => {
          writeSettings("disregardQuicklaunch",v);
          setTimeout(Bangle.drawWidgets,0);
        }
      };
    }

    mainmenu['Force load to launcher'] = {
        value: !!settings.autoloadLauncher,
        onchange: v => {
          writeSettings("autoloadLauncher",v);
          if (v && settings.useAppHistory) {
            writeSettings("useAppHistory",!v);
            setTimeout(()=>E.showMenu(buildMainMenu()), 0); // Update the menu so it can be seen if a value was automatically set to false (app history vs load launcher).
          } // Don't use app history and load to launcher together.
          setTimeout(Bangle.drawWidgets,0);
        }
      };

    mainmenu['Hide "Fastloading..."'] = {
        value: !!settings.hideLoading,
        onchange: v => {
          writeSettings("hideLoading",v);
        }
      };

    mainmenu['This is here so the scroller is active'] = {
        value: !!settings.hideLoading,
        onchange: v => {
          writeSettings("hideLoading",v);
        }
      };

    return mainmenu;
  }

  E.showMenu(buildMainMenu());
}

@gfwilliams
Copy link
Member

Thanks - I suppose that's a solution if it were built-in... Maybe the menu could detect if the menu item was partially offscreen and if so call drawWidgets?

Please don't start committing changes where we add setTimeout(Bangle.drawWidgets,0); to settings code though. This is something that'll need to be fixed in the firmware itself

@thyttan
Copy link
Collaborator Author

thyttan commented Apr 4, 2023

Please don't start committing changes where we add setTimeout(Bangle.drawWidgets,0); to settings code though.

OK, will remove that in my PR for Fastload Utils. 👍

@thyttan
Copy link
Collaborator Author

thyttan commented Apr 4, 2023

Would doing something that involves g.setClipRect maybe be closeer to ideal? So maybe:

// ...
if (isEntryBehindWidgets) { 
g.setClipRect(theAreaOfTheEntryNotBehindWidgets);
}
// ...

is added to the onchange draw logic for entries with checkboxes?

Or just g.setClipRect(0,24,175,175)?

thyttan added a commit to thyttan/Espruino that referenced this issue Apr 4, 2023
The arguments should probably not be hardcoded.

No extensive testing done yet, but seems to work in the case I've tested.

One solution to espruino/BangleApps#2670 (comment)
@thyttan
Copy link
Collaborator Author

thyttan commented Apr 4, 2023

Tried modifying E.showMenu.

To test, copy and upload to ram:

E.showMenu = (function(menu) {
  const H = 40;
  if (menu===undefined) {
    g.clearRect(Bangle.appRect);
    return Bangle.setUI();
  }
  var menuIcon = "\0\f\f\x81\0\xFF\xFF\xFF\0\0\0\0\x0F\xFF\xFF\xF0\0\0\0\0\xFF\xFF\xFF";
  var options = menu[""]||{};
  if (!options.title) options.title="Menu";
  var back = options.back||menu["< Back"];  
  var keys = Object.keys(menu).filter(k=>k!=="" && k!="< Back");
  keys.forEach(k => {
    var item = menu[k];
    if ("object" != typeof item) return;
    if ("boolean" == typeof item.value &&
        !item.format)
      item.format = v=>"\0"+atob(v?"EhKBAH//v/////////////5//x//j//H+eP+Mf/A//h//z//////////3//g":"EhKBAH//v//8AA8AA8AA8AA8AA8AA8AA8AA8AA8AA8AA8AA8AA8AA///3//g");
  });
  // Submenu for editing menu options...
  function showSubMenu(item, title) {
    /*if ("number"!=typeof item.value) 
      return console.log("Unhandled item type");*/
    var step = item.step||1;
    if (!item.noList && item.min!==undefined && item.max!==undefined &&
        ((item.max-item.min)/step)<20) {
      // show scrolling menu of options
      E.showScroller({
        h : H, c : (item.max+step-item.min)/step,
        back: show, // redraw original menu
        remove: options.remove,
        scrollMin : -24, scroll : -24, // title is 24px, rendered at -1
        draw : (idx, r) => {
          if (idx<0) // TITLE
            return g.setFont("12x20").setFontAlign(-1,0).drawString(
          menuIcon+" "+title, r.x+12, r.y+H-12);
          g.setColor(g.theme.bg2).fillRect({x:r.x+4,y:r.y+2,w:r.w-8, h:r.h-4, r:5});
          var v = idx*step + item.min;
          g.setColor(g.theme.fg2).setFont("12x20").setFontAlign(-1,0).drawString((item.format) ? item.format(v,1) : v, r.x+12, r.y+H/2);
          g.drawImage(/* 20x20 */atob(v==item.value?"FBSBAAH4AH/gHgeDgBww8MY/xmf+bH/jz/88//PP/zz/88f+Nn/mY/xjDww4AcHgeAf+AB+A":"FBSBAAH4AH/gHgeDgBwwAMYABmAAbAADwAA8AAPAADwAA8AANgAGYABjAAw4AcHgeAf+AB+A"), r.x+r.w-32, r.y+H/2-10);
        },
        select : function(idx) {
          if (idx<0) return; // TITLE
          Bangle.buzz(20);
          item.value = item.min + idx*step;
          if (item.onchange) item.onchange(item.value);
          scr.scroll = l.scroller.scroll; // set scroll to prev position
          show(); // redraw original menu
        }
      });
    } else {
      // show simple box for scroll up/down
      var R = Bangle.appRect;
      var v = item.value;
      g.reset().clearRect(Bangle.appRect);
      g.setFont("12x20").setFontAlign(0,0).drawString(
          menuIcon+" "+title, R.x+R.w/2,R.y+12);
      
      function draw() {
        var mx = R.x+R.w/2, my = 12+R.y+R.h/2, txt = item.format?item.format(v,2):v, s = 30;
        g.reset().setColor(g.theme.bg2).fillRect({x:R.x+24, y:R.y+36, w:R.w-48, h:R.h-48, r:5});
        g.setColor(g.theme.fg2).setFontVector(Math.min(30,(R.w-52)*100/g.setFontVector(100).stringWidth(txt))).setFontAlign(0,0).drawString(txt, mx, my);
        g.fillPoly([mx,my-45, mx+15,my-30, mx-15,my-30]).fillPoly([mx,my+45, mx+15,my+30, mx-15,my+30]);
      }
      draw();
      Bangle.setUI({
          mode: "updown",
          back: show,
          remove: options.remove
        }, dir => {
        if (dir) {
          v -= (dir||1)*(item.step||1);
          if (item.min!==undefined && v<item.min) v = item.wrap ? item.max : item.min;
          if (item.max!==undefined && v>item.max) v = item.wrap ? item.min : item.max;
          draw();
        } else { // actually selected
          item.value = v;
          if (item.onchange) item.onchange(item.value);
          scr.scroll = l.scroller.scroll; // set scroll to prev position
          show(); // redraw original menu
        }
      });
    }
  }
  var l = {
    draw : ()=>l.scroller.draw(),
    scroller : undefined
  };
  var scr = {
    h : H, c : keys.length/*title*/,
    scrollMin : -24, scroll : options.scroll??-24, // title is 24px, rendered at -1
    back : back,
    remove : options.remove,
    draw : (idx, r) => {
      if (idx<0) // TITLE
        return g.setColor(g.theme.fg).setFont("12x20").setFontAlign(-1,0).drawString(
          menuIcon+" "+options.title, r.x+12, r.y+H-12);
      let R = Bangle.appRect;
      g.setClipRect(R.x,R.y,R.x2,R.y2).setColor(g.theme.bg2).fillRect({x:r.x+4, y:r.y+2, w:r.w-8, h:r.h-4, r:5});
      g.setColor(g.theme.fg2).setFont("12x20");
      var pad = 24;
      var item = menu[keys[idx]];
      if ("object" == typeof item) {
        var v = item.value;
        if (item.format) v=item.format(v);
        if (g.stringMetrics(v).width > r.w/2) // bodge for broken wrapString with image
          v = g.wrapString(v,r.w/2).join("\n");
        g.setFontAlign(1,0).drawString(v,r.x+r.w-8,r.y+H/2);
        pad += g.stringWidth(v);
      } else if ("function" == typeof item) {
        g.drawImage(/* 9x18 */atob("CRKBAGA4Hg8DwPB4HgcDg8PB4eHg8HAwAA=="), r.x+r.w-21, r.y+H/2-9);
        pad += 16;
      }
      var title = (item&&item.title)??keys[idx];
      var l = g.wrapString(title,r.w-pad);
      if (l.length>1)
        l = g.setFont("6x15").wrapString(title,r.w-pad);
      g.setFontAlign(-1,0).drawString(l.join("\n"), r.x+12, r.y+H/2);
    },
    select : function(idx) {
      if (idx<0) return back&&back(); // title
      var item = menu[keys[idx]];
      Bangle.buzz(20);
      if ("function" == typeof item) item(l);
      else if ("object" == typeof item) {
        // if a bool, just toggle it
        if ("number" == typeof item.value) {
          showSubMenu(item, keys[idx]);
        } else {
          if ("boolean"==typeof item.value)
            item.value=!item.value;
          if (item.onchange) item.onchange(item.value);
          l.scroller.drawItem(idx);
        }
      }
    }
  };
  function show() {
    l.scroller = E.showScroller(scr);
  }
  show();
  return l;
})


(function(back) {
  Bangle.loadWidgets();
  Bangle.drawWidgets();
  var FILE="fastload.json";
  var settings;
  var isQuicklaunchPresent = !!require('Storage').read("quicklaunch.app.js", 0, 1);

  function writeSettings(key, value) {
    var s = require('Storage').readJSON(FILE, true) || {};
    s[key] = value;
    require('Storage').writeJSON(FILE, s);
    readSettings();
  }

  function readSettings(){
    settings = require('Storage').readJSON(FILE, true) || {};
  }

  readSettings();

  function buildMainMenu(){
    var mainmenu = {};

    mainmenu[''] = { 'title': 'Fastload', back: back };

    mainmenu['Activate app history'] = {
        value: !!settings.useAppHistory,
        onchange: v => {
          writeSettings("useAppHistory",v);
          if (v && settings.autoloadLauncher) {
            writeSettings("autoloadLauncher",!v);  // Don't use app history and load to launcher together.
            setTimeout(()=>E.showMenu(buildMainMenu()), 0); // Update the menu so it can be seen if a value was automatically set to false (app history vs load launcher).
          }
        }
      };

    if (isQuicklaunchPresent) {
      mainmenu['Keep Quick Launch out of history'] = {
        value: !!settings.disregardQuicklaunch,
        onchange: v => {
          writeSettings("disregardQuicklaunch",v);
        }
      };
    }

    mainmenu['Force load to launcher'] = {
        value: !!settings.autoloadLauncher,
        onchange: v => {
          writeSettings("autoloadLauncher",v);
          if (v && settings.useAppHistory) {
            writeSettings("useAppHistory",!v);
            setTimeout(()=>E.showMenu(buildMainMenu()), 0); // Update the menu so it can be seen if a value was automatically set to false (app history vs load launcher).
          } // Don't use app history and load to launcher together.
        }
      };

    mainmenu['Hide "Fastloading..."'] = {
        value: !!settings.hideLoading,
        onchange: v => {
          writeSettings("hideLoading",v);
        }
      };

    mainmenu['This is here so the scroller is active'] = {
        value: !!settings.hideLoading,
        onchange: v => {
          writeSettings("hideLoading",v);
        }
      };

    return mainmenu;
  }

  E.showMenu(buildMainMenu());
})()

@gfwilliams
Copy link
Member

Just fixed with: espruino/Espruino@b6f8105

Turns out the scroller drawItem used setClipRect and it should have cropped the coordinates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants