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

Discussion: HW buttons should act on 'rising' edge #3435

Open
thyttan opened this issue May 27, 2024 · 25 comments
Open

Discussion: HW buttons should act on 'rising' edge #3435

thyttan opened this issue May 27, 2024 · 25 comments

Comments

@thyttan
Copy link
Collaborator

thyttan commented May 27, 2024

I recently watched "Theo - t3.gg" discuss a twitter post from John Carmack talking about using edge 'rising' vs 'falling' and user experience. The take home message being to act on the 'rising' edge when it's possible. Here's the video: https://www.youtube.com/watch?v=yaMGtiPckAQ

In the name of responsivness I wonder if it would make sense to adhere to this way of thinking when programming the hardware button on the Bangle.js.

I have personally made changes from 'rising' to 'falling' edge responses as to not leave a 'falling' edge for a subsequent screen to pick up - I was seeing some bug related to this (one reference). Now I find myself wanting to go the other way.

To avoid unexpected/unwanted interactions on a left over 'falling' edge, could some default behaviour of disregarding just the 'falling' edge event if it occurred within a short time after the setWatch was called be implemented? If not in setWatch then maybe in setUI? Or could this be solved by changing the default of reacting on 'both' to instead use 'rising'?

Really I just wanted to discuss if others also thinks this is worthwhile. Maybe it's a lot of work, complicating the logic and not backwards compatible. But I want as much snappiness as possible out of the watch! :P

@thyttan
Copy link
Collaborator Author

thyttan commented May 27, 2024

Probably relevant comment:

Yes, responding on the falling edge looks good - and was certainly the reason for going back 2 levels.

I forget the original reasoning for using falling in Bangle.js (I think someone wanted to be able to detect difference between long/short presses?) but now everything uses falling, so really most apps should try and use that where possible to avoid this issue.

I'd prefer not to change the setWatch default just because it's so low level and shared with other devices, but I think ideally we should just try and use setUI wherever we can and avoid this and the whole isBTN3?BTN3:BTN1 issue (I should really update the tutorials)

Originally posted by @gfwilliams in #3297 (comment)

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 1, 2024

Maybe I can try this out by overwriting setWatch and/or setUI to see how it would behave. If it works I could make it into an app as per the linked tutorial.

@gfwilliams
Copy link
Member

Yes, you could try overwriting those functions and see what happens - overwriting setWatch might be enough, and maybe it's something you could even provide as an app.

But personally I don't think this is something I'd be interested in changing, because of the long-press issue. One of the major issues folks seem to take with the Bangle is the lack of buttons, so removing the ability to get long press info out of the button feels like a bad move.

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 3, 2024

I might give overwriting setWatch a try then.

@thyttan thyttan closed this as not planned Won't fix, can't repro, duplicate, stale Jun 3, 2024
@gfwilliams
Copy link
Member

Now you've bought it up I'm more aware of how long I end up pressing the button for before releasing :)

Try adding:

{
  let _setWatch = setWatch;
  global.setWatch = (fn,pin,opt) => {
    if (opt && opt.edge && opt.edge!="both") opt.edge = "rising";
    return _setWatch(fn, pin, opt);
  };
}

to custom boot code (or any file called xyz.boot.js on the Bangle.

It doesn't immediately break things, and it does feel snappier after. If it doesn't screw stuff up too much it might be worth making it a 'beta' app...

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 3, 2024

Thanks! I'll check that out :)

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 3, 2024

Ok - yes my watch feels substantially much more snappy! :)

@thyttan thyttan reopened this Jun 3, 2024
@thyttan
Copy link
Collaborator Author

thyttan commented Jun 7, 2024

Compilation of APP - PROBLEM when running the boot code from #3435 (comment):

  • runplus - Long-press to exit app will toggle start/stop state (or trigger a prompt menu).
  • ... will add as I stumble upon more problems...

Ideas for fixing/working around problems

  1. setWatch defaults to "rising" edge (the reference erroneously says default is "both"). Make Bangle.setUI and the Layout module etc. also default to "rising" , but also provide the option to explicitly choose what edge to use.
  2. Implement a blacklist for disabling the suggested beta-app on an per app basis. The blacklist can even be hardcoded into the app as to not need to require("Storage") to check some file for the list.

thyttan pushed a commit to thyttan/BangleApps that referenced this issue Jun 8, 2024
thyttan pushed a commit to thyttan/BangleApps that referenced this issue Jun 8, 2024
thyttan pushed a commit to thyttan/BangleApps that referenced this issue Jun 8, 2024
thyttan pushed a commit to thyttan/BangleApps that referenced this issue Jun 8, 2024
thyttan pushed a commit to thyttan/BangleApps that referenced this issue Jun 8, 2024
@gfwilliams
Copy link
Member

the reference erroneously says default is "both"

Actually it does say:

see below for more information

and then

edge:'rising'(default for built-in buttons)/'falling'/'both'(default for pins),

but I'll delete the initial default comment

Make Bangle.setUI and the Layout module etc. also default to "rising" , but also provide the option to explicitly choose what edge to use.

My concern is we'll end up with other places we need to add all these little hacks too. Perhaps we can do something like Bangle.disableRisingEdge&&Bangle.disableRisingEdge() to apps that have the issue.

However I think the long press to get home issue is probably going to be a much bigger deal. I mean literally any app that uses the button for anything except exiting to the clock will suffer from this problem if someone long-presses the button, and I wonder if there's really a way around it?

I'm sure someone will arge that having some apps respond on one edge and others respond on the other is confusing as well.

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 10, 2024

Yes, very sound arguments. I'm happy with just using the boot code you posted above for a bit. Not sure if we should do system wide changes either but the snappiness is nice 😉

Edit: thank's for pointing out the docs. Sorry for not reading thoroughly..

@gfwilliams
Copy link
Member

Sorry for not reading thoroughly..

Well, thanks for pointing out the issue. Most people (including me) only glance at the docs anyway, so we don't want the summary up top to be wrong.

Thinking about it, the cases where we could respond quickly and it wouldn't be a problem are where the button press effectively does what a long press would do anyway/doesn't really have an effect, so:

  • when used for back
  • in a clock when going to the launcher

And I guess those handle a good 80% of use cases anyway?

So maybe rather than overriding setWatch it makes sense to override setUI so that in the back case it uses the rising edge? https://github.com/espruino/Espruino/blob/master/libs/js/banglejs/Bangle_setUI_Q3.js#L136

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 10, 2024

Yes that sounds like it would be mostly problem free!

I guess some app may add additional button watches after a setUI call which may be problematic. Don't have an example right now though. Edit: but that shiuldn't really work today either right?

@gfwilliams
Copy link
Member

I guess some app may add additional button watches after a setUI call which may be problematic.

True... In a way it'd be good to know those so they could be changed to use setUI for everything...

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 10, 2024

I guess some app may add additional button watches after a setUI call which may be problematic.

True... In a way it'd be good to know those so they could be changed to use setUI for everything...

Searching files that contain setWatch\((.+)BTN(.+)\) and then among the results find the ones that contain Bangle\.setUI\( and put in quick fix list:

apps/spotrem/app.js|104 col 3| Bangle.setUI(
apps/setting/settings.js|870 col 3| Bangle.setUI();
apps/intervalTimer/app.js|299 col 3| Bangle.setUI(); // remove all existing input handlers
apps/tempmonitor/tempmonitor.app.js|265 col 1| Bangle.setUI({
apps/score/score.app.js|46 col 3| Bangle.setUI('updown', v => {
apps/timerclk/app.js|179 col 1| Bangle.setUI("clock"); // Show launcher when middle button pressed
apps/openstmap/app.js|243 col 3| Bangle.setUI({mode:"custom",drag:e=>{
apps/mixdiganclock/mixdiganclock.app.js|31 col 67| if (require("Storage").read("messagelist.app.js")===undefined)  Bangle.setUI("clock"); // implies btn2(js1)  btn(js2)- launcher
apps/calculator/app.js|415 col 3| Bangle.setUI({ 
apps/hworldclock/app.js|434 col 1| Bangle.setUI({
apps/nixie/app.js|421 col 3| Bangle.setUI("clock");
apps/podadrem/app.js|105 col 3| Bangle.setUI(
apps/helloworld/helloworld.app.js|210 col 7| Bangle.setUI({
apps/astral/app.js|853 col 1| Bangle.setUI("clock");
apps/heartzone/app.js|76 col 1| Bangle.setUI('updown', function(action) {
apps/binwatch/app.js|336 col 1| Bangle.setUI("clock");
apps/mclockplus/mclockplus.app.js|308 col 1| Bangle.setUI("clock");

modules/Layout.js|81 col 3| Bangle.setUI(); // remove all existing input handlers
modules/Layout.js|86 col 5| Bangle.setUI({mode:"updown", back:this.options.back, remove:this.options.remove}, dir=>{
modules/Layout.js|103 col 61| if ((this.options.back || this.options.remove) && !uiSet) Bangle.setUI({mode: "custom", back: this.options.back, remove: this.options.remove});
modules/Layout.min.js|3 col 348| h,b,f,a){var e=null==d.bgCol?a:g.toColor(d.bgCol);if(e!=a||"txt"==d.type||"btn"==d.type||"img"==d.type||"custom"==d.type){var l=d.c;delete d.c;var k="H"+E.CRC32(E.toJS(d));l&&(d.c=l);delete h[k]||((f[k]=[d.x,d.y,d.x+d.w-1,d.y+d.h-1]).bg=null==a?g.theme.bg:a,b&&(b.push(d),b=null))}if(d.c)for(var c of d.c)t(c,h,b,f,e)}p.prototype.setUI=function(){Bangle.setUI();let d;this.buttons&&(Bangle.setUI({mode:"updown",back:this.options.back,remove:this.options.remove},h=>{var b=this.selectedButton,f=this.buttons.length;
modules/Layout.min.js|4 col 302| if(void 0===h&&this.buttons[b])return this.buttons[b].cb();this.buttons[b]&&(delete this.buttons[b].selected,this.render(this.buttons[b]));b=(b+f+h)%f;this.buttons[b]&&(this.buttons[b].selected=1,this.render(this.buttons[b]));this.selectedButton=b}),d=!0);!this.options.back&&!this.options.remove||d||Bangle.setUI({mode:"custom",back:this.options.back,remove:this.options.remove});if(this.b){function h(b,f){.75<f.time-f.lastTime&&this.b[b].cbl?this.b[b].cbl(f):this.b[b].cb&&this.b[b].cb(f)}Bangle.btnWatches&&

Searching files that contain Bangle\.setUI\( and then among the results find the ones that contain setWatch\((.+)BTN(.+)\) and put in quick fix list:

apps/tempmonitor/tempmonitor.app.js|5 col 20| SetUI, Layout, and setWatch( function(b) { }, BTN1, { repeat: true, edge:'falling' })
apps/mixdiganclock/mixdiganclock.app.js|32 col 57| else if (v_model=='BANGLEJS'||v_model=='EMSCRIPTEN') setWatch(function (){load("messagelist.app.js");}, BTN2, { repeat: true });   
apps/mixdiganclock/mixdiganclock.app.js|33 col 12| else setWatch(function (){load("messagelist.app.js");}, BTN1, { repeat: true });
apps/mixdiganclock/mixdiganclock.app.js|35 col 54| else if (v_model=='BANGLEJS'||v_model=='EMSCRIPTEN') setWatch(function (){load("messagegui.app.js");}, BTN2, { repeat: true });   
apps/mixdiganclock/mixdiganclock.app.js|36 col 14| else setWatch(function (){load("messagegui.app.js");}, BTN1, { repeat: true });
apps/mixdiganclock/mixdiganclock.app.js|41 col 4| setWatch(changeFGcolor, BTN1, { repeat: true });        
apps/mixdiganclock/mixdiganclock.app.js|42 col 4| setWatch(changeBGcolor, BTN3, { repeat: true });             
apps/score/score.app.js|57 col 5| setWatch(() => handleInput(2), isBangle1 ? BTN2 : BTN, { repeat: true });
apps/spotrem/app.js|119 col 23| let buttonHandler = setWatch(()=>{load();}, BTN, {edge:'falling'});
apps/openstmap/app.js|281 col 1| setWatch(() => writeSettings(), BTN, { repeat: true, edge: "rising" });
apps/intervalTimer/app.js|129 col 11| //setWatch(startTimer, (process.env.HWVERSION==2) ? BTN1 : BTN2);
apps/podadrem/app.js|120 col 23| let buttonHandler = setWatch(()=>{load();}, BTN, {edge:'falling'});
apps/binwatch/app.js|294 col 5| setWatch(toggleDateTime, BTN1, { repeat : true, edge: "falling"});
apps/nixie/app.js|380 col 3| //setWatch(E.showLauncher, BTN1, {repeat:true,edge:"falling"});
apps/timerclk/app.js|146 col 3| setWatch(()=>load("timerclk.stopwatch.js"), BTN4);
apps/timerclk/app.js|147 col 3| setWatch(()=>load("timerclk.timer.js"), BTN5);
apps/timerclk/app.js|148 col 3| setWatch(()=>load("timerclk.alarm.js"), BTN3);
apps/timerclk/app.js|149 col 3| setWatch(()=>load("timerclk.alarm.js"), BTN1);
apps/calculator/app.js|400 col 3| setWatch(_ => moveDirection(0), BTN1, {repeat: true, debounce: 100});
apps/calculator/app.js|401 col 3| setWatch(_ => moveDirection(2), BTN3, {repeat: true, debounce: 100});
apps/calculator/app.js|402 col 3| setWatch(_ => moveDirection(3), BTN4, {repeat: true, debounce: 100});
apps/calculator/app.js|403 col 3| setWatch(_ => moveDirection(1), BTN5, {repeat: true, debounce: 100});
apps/calculator/app.js|404 col 3| setWatch(_ => buttonPress(selected), BTN2, {repeat: true, debounce: 100});
apps/helloworld/helloworld.app.js|194 col 8| setWatch(ChangeLang, BTN1, { repeat: true });//func  to quit
apps/helloworld/helloworld.app.js|195 col 9| setWatch(ChangeColor, BTN2, { repeat: true });
apps/helloworld/helloworld.app.js|196 col 9| setWatch(Bangle.showLauncher, BTN3, { repeat: true });  
apps/helloworld/helloworld.app.js|198 col 11| else setWatch(ChangeColor, BTN1, { repeat: true });//func  to quit
apps/mclockplus/mclockplus.app.js|245 col 17| if (!w1) w1 = setWatch(resetStopWatch, BTN1, {repeat:false,edge:"falling"});
apps/mclockplus/mclockplus.app.js|280 col 5| setWatch(() => {swInterval=setInterval(stopWatch, 1000);stopWatch();}, BTN3, {repeat:false,edge:"falling"});
apps/mclockplus/mclockplus.app.js|316 col 1| setWatch(() => {swInterval=setInterval(stopWatch, 1000);stopWatch();}, BTN3, {repeat:false,edge:"falling"});
apps/astral/app.js|872 col 1| setWatch(SwitchSensorState, BTN1, { repeat: true });
apps/astral/app.js|874 col 3| setWatch(autoUpdate, BTN3, { repeat: true });
apps/hworldclock/app.js|349 col 6| //setWatch(xxx, BTN1, { repeat: true, debounce:50 }); // maybe adding this later
apps/hworldclock/app.js|350 col 6| //setWatch(xxx, BTN3, { repeat: true, debounce:50 });
apps/hworldclock/app.js|351 col 6| //setWatch(xxx, BTN4, { repeat: true, debounce:50 });
apps/hworldclock/app.js|352 col 6| //setWatch(xxx, BTN5, { repeat: true, debounce:50 });
apps/heartzone/app.js|85 col 1| setWatch(function() { Bangle.setHRMPower(false, "heartzone"); load(); }, BTN1);
apps/setting/settings.js|848 col 5| setWatch(showAppSettingsMenu, BTN1, { repeat: false });

modules/Layout.js|115 col 43| if (this.b[0]) Bangle.btnWatches.push(setWatch(pressHandler.bind(this,0), BTN1, {repeat:true,edge:-1}));
modules/Layout.js|116 col 43| if (this.b[1]) Bangle.btnWatches.push(setWatch(pressHandler.bind(this,1), BTN2, {repeat:true,edge:-1}));
modules/Layout.js|117 col 43| if (this.b[2]) Bangle.btnWatches.push(setWatch(pressHandler.bind(this,2), BTN3, {repeat:true,edge:-1}));
modules/Layout.min.js|5 col 94| Bangle.btnWatches.forEach(clearWatch);Bangle.btnWatches=[];this.b[0]&&Bangle.btnWatches.push(setWatch(h.bind(this,0),BTN1,{repeat:!0,edge:-1}));this.b[1]&&Bangle.btnWatches.push(setWatch(h.bind(this,1),BTN2,{repeat:!0,edge:-1}));this.b[2]&&Bangle.btnWatches.push(setWatch(h.bind(this,2),BTN3,{repeat:!0,edge:-1}))}if(2==process.env.HWVERSION){function h(b,f){b.cb&&f.x>=b.x&&f.y>=b.y&&f.x<=b.x+b.w&&f.y<=b.y+b.h&&(2==f.type&&b.cbl?b.cbl(f):b.cb&&b.cb(f));b.c&&b.c.forEach(a=>h(a,f))}Bangle.touchHandler=

So there they are, seems to be 17 apps + the layout module (given I didn't mess up the regex!). No promises on changing all of them soon though. I'll reply here when/if I start.

(I used this to compile the lists: https://stackoverflow.com/a/77985695)

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 11, 2024

@gfwilliams

btn : function(n) {}, // optional - (mode:custom only) handler for 'button' events (n==1 on Bangle.js 2, n==1/2/3 depending on button for Bangle.js 1) (reference)

So in cases where setUI uses a mode other than "custom" it's not trivial to just move the hw button logic inside setUI. E.g. spotrem uses mode:"updown" and then calls setWatch to add the button handler.

// Navigation input on the main layout
let setUI = function() {
// Bangle.setUI code from rigrig's smessages app for volume control: https://git.tubul.net/rigrig/BangleApps/src/branch/personal/apps/smessages/app.js
  Bangle.setUI(
    {mode : "updown",
      remove : ()=>{
        Bangle.removeListener("touch", touchHandler);
        Bangle.removeListener("swipe", swipeHandler);
        clearWatch(buttonHandler);
        widgetUtils.show();
      }
    },
      ud => {
        if (ud) Bangle.musicControl(ud>0 ? "volumedown" : "volumeup");
      }
  );
  Bangle.on("touch", touchHandler);
  Bangle.on("swipe", swipeHandler);
  let buttonHandler = setWatch(()=>{load();}, BTN, {edge:'falling'});
};

https://github.com/espruino/BangleApps/blob/fd7ea9a587d3a5a083283d24eb0b04296f4e1e7b/apps/spotrem/app.js#L101C1-L120C3

@gfwilliams
Copy link
Member

I think in that case, doesn't the button end up calling the function with ud==0?

We could extend setUI so that even if mode!=custom, button/touch/swipe/etc are still honoured?

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 11, 2024

I think in that case, doesn't the button end up calling the function with ud==0?

Sorry, I changed/messed up the code before commenting - I updated the code block above now to reflect the actual code in the repo. There's no btn : ... in there - it was just me thinking... The callback function is for the up/down-swipes.

We could extend setUI so that even if mode!=custom, button/touch/swipe/etc are still honoured?

That's what I'm leaning toward as well. It's nice being able to use the preconfigured behaviours but add on to them when you want to.

@gfwilliams
Copy link
Member

If you do:

      ud => {
        if (ud) Bangle.musicControl(ud>0 ? "volumedown" : "volumeup");
        else load();
      }

I think that has mostly the same effect looking at setUI - although there is an issue that touching the screen would take you back.

I'd be happy to change setUI as it should be backwards compatible, but obviously any apps you change wouldn't work unless you had the newer Bangle.setUi version

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 11, 2024

I'd be happy to change setUI as it should be backwards compatible, but obviously any apps you change wouldn't work unless you had the newer Bangle.setUi version

OK! Maybe we update setUI soon-ish and wait with merging updates to the apps until after stable fw 2v23 is out?

@gfwilliams
Copy link
Member

Sounds like a good plan, yes

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 11, 2024

Added an example on how we might be able to interact with setUI after an update of it: #3452

@bobrippling
Copy link
Collaborator

Compilation of APP - PROBLEM when running the boot code from #3435 (comment):

* `runplus` - Long-press to exit app will toggle start/stop state (or trigger a prompt menu).

I guess this is somewhat similar to the E.stopEventPropagation(), except we don't have that for the button press logic. Perhaps we could have it work for buttons too?

We could extend setUI so that even if mode!=custom, button/touch/swipe/etc are still honoured?

I found I needed a case like this recently, can't remember the app now though

@gfwilliams
Copy link
Member

Perhaps we could have it work for buttons too?

The problem is that the whole point is to do the action as soon as possible, so the action is performed, but then ~1 sec later for the long press we exit the app. We can't use E.stopEventPropagation() as the event has already happened - we'd actually have to undo the command.

I guess for run/runplus the solution would be to swap the visible recording state when the button was pressed so it looked fast, but to only actually make the change when the button was released? But the actual visible change is so minimal I think it probably makes sense just to explicitly request the falling edge

@thyttan
Copy link
Collaborator Author

thyttan commented Jun 17, 2024

Just a note to say I don't want to rush this. There's obviously some deliberations to be had so we can land on a good solution.

thyttan pushed a commit to thyttan/BangleApps that referenced this issue Jun 30, 2024
thyttan added a commit to thyttan/Espruino that referenced this issue Jul 1, 2024
TODO: Not thoroughly tested yet.

First discussed here around here:
espruino/BangleApps#3435 (comment)
thyttan added a commit to thyttan/Espruino that referenced this issue Jul 1, 2024
To make the ui feel snappier.

First discussed around here:
espruino/BangleApps#3435 (comment)
thyttan pushed a commit to thyttan/BangleApps that referenced this issue Jul 2, 2024
To make the ui feel snappier.

First discussed around here:
espruino#3435 (comment)
thyttan pushed a commit to thyttan/BangleApps that referenced this issue Jul 2, 2024
@gfwilliams
Copy link
Member

Just posted about this on the forum: https://forum.espruino.com/conversations/397606/

Also I realised Layout detects long presses and could be an issue, but it uses setWatch directly so is ok: https://github.com/espruino/BangleApps/blob/master/modules/Layout.js#L107-L117

It could detect if there's a no cbl handler and callback on the first press, but given it's not used so much I think we're probably ok leaving as-is for now.

thyttan pushed a commit to thyttan/BangleApps that referenced this issue Jul 6, 2024
To make the ui feel snappier.

First discussed around here:
espruino#3435 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants