Add Range actions #5052

pmario · 2020-11-16T11:37:54Z

This is WIP and contains some console.log() functions so you can see, what's going on with F12

There some redundant code to simplify debugging. ... See code-comments!

This PR adds 4 actions:

actionsMouseDown
actionsMouseUp
actionsInput
actionsChange

It behaves the same way in modern browsers and contains some lines of code to behave well with IE11.
I did test it with IE11 wondows 10, FF latest, Edge latest. ..

Go to Latest Version:

…t output

pmario · 2020-11-16T11:45:06Z

This PR will fix: [IDEA] Add actions parameter to "range widget" #5029

Jermolene

Thanks @pmario

Jermolene · 2020-11-16T16:44:24Z

core/modules/widgets/range.js

@@ -92,16 +173,30 @@ RangeWidget.prototype.handleInputEvent = function(event) {
 Compute the internal state of the widget
 */
 RangeWidget.prototype.execute = function() {
+	// TODO remove the next 2 lines once IE is gone!
+	this.mouseUp = true; // Needed for IE10
+	this.isIE = (/msie|trident/i.test(window.navigator.userAgent)) ? true : false;


The core already defines $tw.browser.isIE

Jermolene · 2020-11-16T16:45:14Z

core/modules/widgets/range.js

+	this.actionsMouseUp = this.getAttribute("actionsMouseUp","");
+	// Change only fires, if start-value is different to end-value
+	this.actionsChange = this.getAttribute("actionsChange","");
+	// input fires very often!!


It's a small thing but we do try to capitalise comments as sentence case to keep things neat

Jermolene · 2020-11-16T16:45:40Z

core/modules/widgets/range.js

+	// Next 3 only fire once!
+	this.actionsMouseDown = this.getAttribute("actionsMouseDown","");
+	this.actionsMouseUp = this.getAttribute("actionsMouseUp","");
+	// Change only fires, if start-value is different to end-value


The comma isn't necessary here

Jermolene · 2020-11-16T16:46:38Z

core/modules/widgets/range.js

@@ -111,15 +206,17 @@ Selectively refreshes the widget if needed. Returns true if the widget or any of
 */
 RangeWidget.prototype.refresh = function(changedTiddlers) {
 	var changedAttributes = this.computeAttributes();
-	if(changedAttributes.tiddler || changedAttributes.field || changedAttributes.index || changedAttributes['min'] || changedAttributes['max'] || changedAttributes['increment'] || changedAttributes["default"] || changedAttributes["class"] || changedAttributes.disabled) {
+	if(	changedAttributes.tiddler || changedAttributes.field || changedAttributes.index || changedAttributes['min'] || changedAttributes['max'] ||


I don't mind the condition being split to multiple lines, but we wouldn't usually do it by adding whitespace between the opening parenthesis and the condition.

Using tab characters for alignment (the "if(" line is really "if(→" where → represents a tab character) is a bad idea, because it ensures that the code will only be aligned if someone else uses tab settings that are the same size as yours, thereby removing the one big advantage of tabs for indentation (that people can set their own tab settings). Here you have the right idea by putting a tab after the "if(", so that people with either 4-space or 8-space tab settings will see the code correctly aligned. But if someone has their tab size set to 2 spaces, then this is no longer aligned.

If you are using tabs for indentation, the only correct approach is "tabs for indentation, spaces for alignment". The second line of the if( condition should be indented the same level as the if( (here that's two tab characters for two indentation levels), with three spaces after the two tab characters to line up the changedAttributes text. I.e., the second line should look like →→···changedAttributes['increment'], where → represents a tab character and · represents a space. That is the only correct way to do alignment when you have used tabs for indentation, because it ensures that the aligned lines will stay aligned in everyone's editor no matter what tab settings they use.

And the big problem with "tabs for indentation, spaces for alignment" is that most editors are not smart about it, and will mess it up. Which means that if you've aligned your code, you then have to pay careful attention to your whitespace every time you edit that line. Which is a lot of fiddly work that I, for one, don't care for, and that's why my personal preference is to use space characters, not tabs, for indentation.

There are really only four options:

Use tab characters naively, for both indentation and alignment, and end up with code that looks different when different editor settings are applied.

Use tab characters intelligently, using tabs for indentation and spaces for alignment, and end up paying far more attention to whitespace than it's worth.

Use tab characters for indentation and forget trying to align subsequent lines. The rule for function parameters, second lines of "if()" conditions, and so on, is "just indent one more level with one extra tab character". This will still look reasonably good on everyone's editor, and everyone can use their favorite indentation level by tweaking their editor's tab settings.

Use space characters for indentation. This allows you to align function parameters, "if()" conditions, and so on, but other coders won't get to use their favorite indentation level and must conform to the indentation level the project has chosen.

My personal preference is for 4. 1 is just bad, 2 causes you to pay way more attention to whitespace than it's worth, and 3 sacrifices alignment, which is a useful tool in many situations. So I'd rather choose option 4 and sacrifice the ability to use my preferred indentation, instead using whatever the project has settled on.

Point is, it's up to you whether you choose 2 or 3 here (4 is not an option since the TiddlyWiki project uses tabs for indentation), but please don't choose option 1. To choose option 2, write your continuation lines as →→···condition. To choose option 3, write your continuation lines as →→→condition.

Use space characters for indentation. This allows you to align function parameters, "if()" conditions, and so on, but other coders won't get to use their favorite indentation level and must conform to the indentation level the project has chosen.

TW doesn't compress or minify the source code. That makes it easy for every user to start to read the code and start to debug it.

That means the empty.html would contain 4 times more indentation spaces as used with tabs. This would considerably increase the basic file size. So atm TW uses tabs for a reason.

Indeed, TW is one of the few projects where the fact that tabs take less storage than spaces is actually relevant. Most of the time compression makes the space savings of tab characters irrelevant, but in TW it actually matters. I wasn't actually arguing that TW should change (I did mention that option 4 is not an option in this case), I was just trying to list all available options for dealing with indentation & alignment. I agree that using tabs for TW makes sense since it isn't minified (though it probably is compressed, given that most servers default to gzipping HTML for delivery).

Though it is actually a little hard for every user to read the code, since when you download TW in HTML form via the big "Download Empty" button, the HTML form has had to store the code in a way that looks like "{\n\t\t\t\tresults.push(title);\n\t\t\t}" and so on. Granted, anyone who knows enough about Javascript to want to modify that code also probably knows how to do a search-and-replace to turn "\n" into a real newline and "\t" into a real tab. But there is a small barrier to entry; it's not quite the ideal situation that you described. :-)

rmunn · 2020-11-17T06:56:45Z

core/modules/widgets/range.js

+		{name:"mousedown", handlerObject:this, handlerMethod:"handleMouseDownEvent"},
+		{name:"mouseup",   handlerObject:this, handlerMethod:"handleMouseUpEvent"},
+		{name:"change",    handlerObject:this, handlerMethod:"handleChangeEvent"},
+		{name:"input",     handlerObject:this, handlerMethod:"handleInputEvent"},


I'd rather see this list built dynamically based on whether there's an attribute specified for that event. Something like this:

var eventList = []; if(this.actionsMouseDown) { eventList.push({name:"mousedown", handlerObject:this, handlerMethod:"handleMouseDownEvent"}); } // Similar for this.actionsMouseUp and this.actionsChange if(this.actionsInput) { eventList.push({name:"input", handlerObject:this, handlerMethod:"handleInputEvent"}); } $tw.utils.addEventListeners(this.inputDomNode, eventList);

The advantage is that if the end user doesn't care about the input event, we don't need to call a dozen times into an event handler that will end up doing nothing. There is a performance cost to the browser firing events, even if the event handler returns quickly. If we can avoid hooking up the event at all, the browser can optimize and not fire the event because it knows nothing is going to handle the event.

Good point @rmunn

The problem with input and change event are, that they have to be fired, even if there is no action. Otherwise the target tiddler wouldn't be updated. It doesn't matter if there are actions defined or not. ... Input is fired on all modern browsers and change is fired in IE.

You are right for MouseDown and MouseUp. ATM they are only there for handling actions atm. ... BUT we don't know if this will change in the future.

Events are only fired if user wants interaction. So IMO there is no real performance overhead, without user interaction. So I'd vote for simple code here.

I forgot something. MouseUp is needed for IE to trigger the change event. So it doesn't matter if actions are defined.

pmario · 2020-11-17T21:19:47Z

@Jermolene Should I add the prepareAttributs to the basic widget.js? ... It can be made more generic and may be renamed to .prepareActionAttributes(). So we can probably reduce code duplication.

https://github.com/Jermolene/TiddlyWiki5/pull/5052/files#diff-10dbab80ae6e1067219cf5b2d25d69fd68bc382029e3056c37feb20ccf4d565cR83

saqimtiaz · 2020-11-17T21:21:40Z

@pmario I think a method on the base widget class makes sense.

Perhaps getActionVariables()?

pmario · 2020-11-17T21:29:52Z

Latest commit 8b25443 uses actionValue instead of attr-value now. .. Will add actionValue for radio widget too.

saqimtiaz · 2020-11-17T21:36:27Z

core/modules/widgets/range.js

@@ -111,15 +202,17 @@ Selectively refreshes the widget if needed. Returns true if the widget or any of
 */
 RangeWidget.prototype.refresh = function(changedTiddlers) {
 	var changedAttributes = this.computeAttributes();
-	if(changedAttributes.tiddler || changedAttributes.field || changedAttributes.index || changedAttributes['min'] || changedAttributes['max'] || changedAttributes['increment'] || changedAttributes["default"] || changedAttributes["class"] || changedAttributes.disabled) {
+	if(changedAttributes.tiddler || changedAttributes.field || changedAttributes.index || changedAttributes['min'] || changedAttributes['max'] ||


@pmario if we need to call refreshSelf() if any attributes have changed, it is easier to just check the count:

if($tw.utils.count(changedAttributes) > 0) { this.refreshSelf(); return true; }

Isn't this the case for every widget?

@pmario We just can't use this trick when we only want to call refreshSelf() on some attributes changing but not for others. So there is probably room for some refactoring and optimization in other core widgets.

saqimtiaz · 2020-11-17T21:40:13Z

core/modules/widgets/range.js

+	// Trigger actions
+	if(this.actionsInput) {
+		// "tiddler" parameter may be missing. See .execute() below
+		var variables = this.prepareAttributes() // TODO this line will go into the function call below.


currently handleEvent() does debouncing and only calls wiki.setText if the value of the widget has changed. We should similarly debounce the actions, at least the ones for events like input that fire very frequently:

if(this.getValue() !== this.inputDomNode.value) { this.invokeActionString(this.actionsInput,this,event,variables); }

input or change(on IE) are only fired, if the value has been changed. ... IMO this is no debouncing.

The line was introduced with commit e84c422 ... But I don't see a difference, if I remove it. ... @Jermolene Do you remember the problem?

@pmario according to the spec you should be right. I thought I ran into some issue with this last I worked on this widget. I can do some tests to confirm tomorrow.

@pmario I can confirm that the debouncing does not seem necessary on the input event. In both Chrome and FF I cannot get the event to fire more than once per change of value.

saqimtiaz · 2020-11-19T08:10:05Z

@pmario @Jermolene @rmunn I have some concerns regarding usability from the point of view of end users, and a question in this regard please:

@rmunn has given some great examples of why having separate actions for the input and change events would be useful. However, what added benefit is there in making the distinction of having separate actions for mouseUp and mouseDown, in addition to actions associated with the change and input events, and how do we reconcile that with the usability perspective?

I am worried about how accessible this will be to non-technical users who need to distinguish between 4 types of actions:

actionsChange
actionsInput
actionsMouseDown
actionsMouseUp

If we are handling those extra events (mousedown and mouseup) internally just for IE compatibility, I suggest we not expose them as widget parameters for distinct actions unless or until there is a specific need to do so.

Furthermore the current attribute names are perhaps too developer centric, especially if there are four of them. These are user facing widget attributes and not internal variables or method names.

Jermolene · 2020-11-19T08:16:11Z

I'm inclined to agree with @saqimtiaz. If we do want to trap mousedown/move/up, then I think we might be better off making a generic event trapping widget that traps events within its content. That widget could be wrapped around any other widget without that widget being aware of the event handling.

saqimtiaz · 2020-11-19T08:21:57Z

I'm inclined to agree with @saqimtiaz. If we do want to trap mousedown/move/up, then I think we might be better off making a generic event trapping widget that traps events within its content. That widget could be wrapped around any other widget without that widget being aware of the event handling.

I like this idea! In many ways the button widget does this for the click event, but is overloaded with other features.

We could generalize this to a trap widget that can trap any type of event we specify and associate actions with them. There is a good case for the dblclick and contextmenu events as well, and this would be far more versatile and flexible than adding support for each event to every widget where it might be needed.

Jermolene · 2020-11-19T08:24:34Z

I'm inclined to agree with @saqimtiaz. If we do want to trap mousedown/move/up, then I think we might be better off making a generic event trapping widget that traps events within its content. That widget could be wrapped around any other widget without that widget being aware of the event handling.

I like this idea! In many ways the button widget does this for the click event, but is overloaded with other features.

We could generalize this to a trap widget that can trap any type of event we specify and associate actions with them. There is a good case for the dblclick and contextmenu events as well, and this would be far more versatile and flexible than adding support for each event to every widget where it might be needed.

I've got a very simple prototype <$event> widget for click events, but <$trap> is a better name. I'll post it in a moment.

Jermolene · 2020-11-19T08:42:28Z

I've got a very simple prototype <$event> widget for click events, but <$trap> is a better name. I'll post it in a moment.

Here's the ticket: #5074

pmario · 2020-11-19T15:08:53Z

I was thinking about it too. So I wanted to implement a actionStart, actionStop and action. Where start gets actionValue as the start-value. Stop gets actionValue and actionValueHasChanged info. So the user has easy access to the info if the start- and end-value are different. 'action' is activated on input, which is fired a lot of times. ...

It's made sure, that "start" and "stop" is only called once. and action is called whenever the handle is moved.

pmario · 2020-11-20T01:28:54Z

Go To Latest Version

It has 3 actions now:

actionsStart ... fires 1x if range-handle is clicked
actionsStop ... fires 1x if handle is released
actions ... fires if handle is moved.

actionValue is available in the actions
actionValueHasChanged can be "yes" / "no" and shows, if the value between start and stop has changed. In actions it is always "yes", since that action is only fired if the value has been changed, between 2 calls.

pmario · 2020-11-22T16:19:55Z

Latest Version: range-actions.zip

All console.log() and redundant code is removed. see commit: 0111333

saqimtiaz · 2020-11-22T16:23:10Z

core/modules/widgets/range.js

 			}
 		}
 	}
 	return value;
 };

+RangeWidget.prototype.prepareAttributes = function(options) {


IMHO this name is not intuitive. In fact we are processing attributes to prepare variables, rather than preparing attributes.

Perhaps getActionVariables() so that the context is easier to undertand?

getActionVariables would be more consistent with other usages in the core.

I was getting some inconsistencies with filter operators that expect strings.

twMat · 2020-11-28T21:48:50Z

Bump.
Would be nice if included in 5.1.23

pmario · 2020-11-30T18:59:29Z

Will close this one and create a new and simplified version.

pmario added 30 commits January 4, 2020 11:57

add a new-line before the log text to increase readability of the tes…

090e812

…t output

make eslint, jslint happy

5f35bab

Merge remote-tracking branch 'jermolene/master'

4d610fa

Merge remote-tracking branch 'jermolene/master'

4f68732

Merge remote-tracking branch 'jermolene/master'

c0d4507

Merge remote-tracking branch 'jermolene/master'

19e9eff

Merge remote-tracking branch 'jermolene/master'

8f5b635

Merge remote-tracking branch 'jermolene/master'

15f79e1

Merge remote-tracking branch 'jermolene/master'

789e6fb

Merge remote-tracking branch 'jermolene/master'

f39eaf1

Merge remote-tracking branch 'jermolene/master'

b97cf82

Merge remote-tracking branch 'jermolene/master'

c1448df

it shouldn't be there

a30852a

fremove this file from my PRs

b63b602

Merge remote-tracking branch 'jermolene/master'

bc192f8

Merge remote-tracking branch 'jermolene/master'

611da36

Merge remote-tracking branch 'jermolene/master'

9f8d930

Merge remote-tracking branch 'jermolene/master'

f8789d7

Merge remote-tracking branch 'jermolene/master'

ae84acd

Merge remote-tracking branch 'jermolene/master'

cc07cf8

Merge remote-tracking branch 'origin/master'

4fe0310

Merge remote-tracking branch 'jermolene/master'

31ce0cf

Merge remote-tracking branch 'jermolene/master'

ea29fdb

add a new-line before the log text to increase readability of the tes…

3c9b6f1

…t output

make eslint, jslint happy

bd0b9a3

it shouldn't be there

0589f60

fremove this file from my PRs

34866cb

Merge remote-tracking branch 'origin/master'

313a7ef

Merge remote-tracking branch 'jermolene/master'

7d92bd0

add a new-line before the log text to increase readability of the tes…

83516c8

…t output

a bit of code formatting

299f1f4

Jermolene reviewed Nov 16, 2020

View reviewed changes

rmunn reviewed Nov 17, 2020

View reviewed changes

fix some typos and use $tw.browser.isIE instead of local code

9cc750b

use actionValue instead of attr-value

8b25443

saqimtiaz reviewed Nov 17, 2020

View reviewed changes

Jermolene mentioned this pull request Nov 19, 2020

[IDEA] Add a generic event trapping widget #5074

Closed

reduce actions to actionsStart - actions - actionsStop

9f4b88d

pmario marked this pull request as ready for review November 20, 2020 10:57

remove unused code and console.log()

0111333

saqimtiaz reviewed Nov 22, 2020

View reviewed changes

saqimtiaz referenced this pull request Nov 25, 2020

Fix eventcatcher to ensure variable values are strings

2267e31

I was getting some inconsistencies with filter operators that expect strings.

saqimtiaz mentioned this pull request Nov 28, 2020

Timeline for 5.1.23 release? #5130

Closed

pmario closed this Nov 30, 2020

pmario mentioned this pull request Jan 4, 2021

Introduce shortcut syntax for transclusion passing parameters via variables #5199

Closed

Add Range actions #5052

Add Range actions #5052

Conversation

pmario commented Nov 16, 2020 • edited

pmario commented Nov 16, 2020

Jermolene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmario commented Nov 17, 2020

saqimtiaz commented Nov 17, 2020 • edited

pmario commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saqimtiaz commented Nov 19, 2020

Jermolene commented Nov 19, 2020

saqimtiaz commented Nov 19, 2020 • edited

Jermolene commented Nov 19, 2020

Jermolene commented Nov 19, 2020

pmario commented Nov 19, 2020 • edited

pmario commented Nov 20, 2020 • edited

pmario commented Nov 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twMat commented Nov 28, 2020

pmario commented Nov 30, 2020

pmario commented Nov 16, 2020 •

edited

saqimtiaz commented Nov 17, 2020 •

edited

saqimtiaz commented Nov 19, 2020 •

edited

pmario commented Nov 19, 2020 •

edited

pmario commented Nov 20, 2020 •

edited