Update bgraknob.pas to V2 upgrade

[sganz]

Added numerous event handlers
Added Mouse Wheel support
Added Sector support

See also Forum thread

[lainz]

@circular17 can you check this? It should not be hard to test, just check if everything works fine in LazPaint where you use this control..

[lainz]

@sganz please take care of this, if you can test with LazPaint first, so everything is working fine..

[sganz]

Tested the BGRAKnob with Lazpaint. It exists only in the 3D import. Default values for the v2 knob works fine with defaults. Also tested with Mouse Wheel enabled and works fine. Tested with Slow Snap enabled, works well.

I'll try to do a merge and see how bad I mess up here on git work now.

Sandy

[lainz]

Thanks!

[sganz]

@lainz I don't think I can merge, you may have to do it.

[lainz]

Done

[circular17]

Hi @sganz and @lainz!

Thank you @sganz for the improvements on the knob control! It's great to have new contributors bringing their own perspective. Apologies for the late reply, I have been traveling.

Looking at the changes, I note that there are changes in spacing. I guess you have a tab size of 4 whereas in the file it was a tab size of 2. I do understand that you would like to add spacing before :. It is the convention used in French and arguably it is nice for readability.

I don't really mind changing the spacing, however to visualize the changes in the file, it is easier when it stays the same, so that only the parts that are actually changed are shown. For simplicity, I suggest to set the block indentation to 2 in the options of Lazarus (Tools > Options and then Editor > General > Tabulation and indentation) and to use JEDI formatting at the bottom of the Source menu in Lazarus.

I've applied JEDI formatting. So here are the changes:
dev-bgracontrols@{1day}...dev-bgracontrols

I can see you've added properties and changed a few methods. I like the wheel feature, it seems quite natural to use that, even more so with touchpad scrolling.

Regarding the change in the IFDEF {$if FPC_FULLVERSION < 30203}*{$ELSE}**{$ENDIF} to {$if FPC_FULLVERSION < 030301}*{$ELSE}**{$ENDIF}, it was in fact correct before as far as I can tell. Even though the ** operator was changed in version 3.3.1, this has been ported back to version 3.2.3. Is there any other reason for changing this IFDEF?

Regarding the possible exception in RemapRange, I suggest to make the function more robust. For example, you can add at the beginning:

begin
  if OldMin = OldMax then
  begin
    if OldValue <= OldMin then exit(NewMin) else exit(NewMax);
  end;
  ...

Regarding the ktSector knob type, I find the use of MinValue and MaxValue a bit confusing. These properties seems basically unused from the user point of view and one could want the sector values start from a minimum value different from 0. So I suggest to rethink that a bit. I imagine two possibilities sector option is enabled:

MinValue and MaxValue are limited to integers and SectorDivisions property is removed:
The Value will be an integer and go from MinValue to MaxValue. The SectorDivisions properties can be removed.

• For example: with MinValue = 10 and MaxValue = 90, Value will be an integer from 10 to 90.

MinValue and MaxValue are not limited to integers and SectorDivisions is kept:
SectorDivisions property indicates how many sectors divide the range. Value will go from MinValue to MaxValue according to these sectors, possibly a floating point value.

• For example: with MinValue = 0 and MaxValue = 10 and SectorDivisions = 100, Value will be a number with 1 digit after the decimal point, from 0.0 to 10.0.
• Other example: with MinValue = 0.3 and MaxValue = 0.7 and SectorDivisions = 4, Value will go from 0.3, 0.4, ... to 0.7

What do you think?

[circular17]

We need to think as well about default value of properties. When adding new properties, we can keep the code backward compatible if those new properties have default value that are defined, so that they won't be added to the LFM file if unchanged. That way, when loading with an older version of BGRAControls, there won't be any missing property error.

In all cases, thanks for engaging with us and welcome onboard!

[sganz]

Hi @sganz and @lainz!

Thank you @sganz for the improvements on the knob control! It's great to have new contributors bringing their own perspective. Apologies for the late reply, I have been traveling.

Looking at the changes, I note that there are changes in spacing. I guess you have a tab size of 4 whereas in the file it was a tab size of 2. I do understand that you would like to add spacing before :. It is the convention used in French and arguably it is nice for readability.

I don't really mind changing the spacing, however to visualize the changes in the file, it is easier when it stays the same, so that only the parts that are actually changed are shown. For simplicity, I suggest to set the block indentation to 2 in the options of Lazarus (Tools > Options and then Editor > General > Tabulation and indentation) and to use JEDI formatting at the bottom of the Source menu in Lazarus.

I've applied JEDI formatting. So here are the changes: dev-bgracontrols@{1day}...dev-bgracontrols

I made a mistake at some point and used the code format tool but didn't have a good reference for setting up. I'll look at getting my set up closer to what exists. I really struggle with Pascal formatting :) Do you have a 'jcfsettings.cfg' for the JEDI formatter that you can send so I have exactly the same?

I can see you've added properties and changed a few methods. I like the wheel feature, it seems quite natural to use that, even more so with touchpad scrolling.

Regarding the change in the IFDEF {$if FPC_FULLVERSION < 30203}*{$ELSE}**{$ENDIF} to {$if FPC_FULLVERSION < 030301}*{$ELSE}**{$ENDIF}, it was in fact correct before as far as I can tell. Even though the ** operator was changed in version 3.3.1, this has been ported back to version 3.2.3. Is there any other reason for changing this IFDEF?

I don't know if I changed that, I would have to look at git and see, but I had no intention of changing it, might have been a mistake. The code format tool split the lines up so I manually put them back to a single line. May have botched something, but really didn't try to change that. I think the change was made possibly prior and maybe I just formatted different, but really no intention on changing it.

Regarding the possible exception in RemapRange, I suggest to make the function more robust. For example, you can add at the beginning:

begin
  if OldMin = OldMax then
  begin
    if OldValue <= OldMin then exit(NewMin) else exit(NewMax);
  end;
  ...

Yeah, that's likely better have to update and test but I like doing something better then tossing and exception

Regarding the ktSector knob type, I find the use of MinValue and MaxValue a bit confusing. These properties seems basically unused from the user point of view and one could want the sector values start from a minimum value different from 0. So I suggest to rethink that a bit. I imagine two possibilities sector option is enabled:

MinValue and MaxValue are limited to integers and SectorDivisions property is removed: The Value will be an integer and go from MinValue to MaxValue. The SectorDivisions properties can be removed.

* For example: with MinValue = 10 and MaxValue = 90, Value will be an integer from 10 to 90.

MinValue and MaxValue are not limited to integers and SectorDivisions is kept: SectorDivisions property indicates how many sectors divide the range. Value will go from MinValue to MaxValue according to these sectors, possibly a floating point value.

* For example: with MinValue = 0 and MaxValue = 10 and SectorDivisions = 100, Value will be a number with 1 digit after the decimal point, from 0.0 to 10.0.

* Other example: with MinValue = 0.3 and MaxValue = 0.7 and SectorDivisions = 4, Value will go from 0.3, 0.4, ... to 0.7

What do you think?

I have to think more about the sector. It was a bit of a shoehorn into the knob and gets tricky. The idea of the sectors will still allow the angle positions (start/stop) with fixed start from a value of 0. It's not ideal that the value range from 0-255. The mapping back and forth with setting minValue, and maxValue internally might be tricky, at least as a minimum they need to be integers or the snap to a sector gets more complicated (or really just more work ;) In any case, the first example in removing the sectors and using the integer range between MinValue and MaxValue (your first case above would be same as 80 sectors with changes to the returned Value). This might not be too bad. I think where it gets tricky (In my head) was with mouse and wheel movement snapping to the correct sector. I would say the only think that I would mention is that the setting for sectors is possibly easier to understand then the span of the Min/MaxValue range but I might be thinking different. Let me try the first option and see how that works!

Thanks for the comments and feedback. If you have a configuration file for the JEDI formatter that would be awesome!!

Sandy

[circular17]

Hi Sandy,

I wasn't aware of a specific configuration for the JEDI formatter, as I use the default settings. However, there are lot of options indeed, that's cool. Here is my config file. You may as well get the default seetings by removing the config file.

Regarding the FPC_FULLVERSION check, I understand now that you didn't change it deliberately. It seems the source you started with was from a version before this check was updated to 30203. That's perfectly fine as we have the change history. I confirm that all other changes have been included in your version so you probably had the one of Nov 29, 2023.

I've restored the number to 30203 so it's all set now. The key takeaway here is to always make modifications via the Git repository and preferably with the latest dev version.

About the sectors, I understand it might not be as straightforward to use MinValue and MaxValue. I feel the same however it matters more in my view to avoid duplicate properties. If we were to define a minimum value we would have something like MinSector and MaxSector, which essentially conveys the same information.

I understand there might be some difficulty with handling the wheel. You're welcome to ask any question if needed. Happy experimenting!

Circular

[sganz]

Got the file, I likely could have just got it from a fresh load of Lazarus, but at least it's what you are using and should hopefully not mess up the projects formatting. Might be a good thing down the road to set it up exactly how you like the code to look and save as part of the BGRA project so others can have similar project formatting. It's odd when I run your config and reformat the knob code it does break up the single line of compiler line options that you reverted back (** operator).

Thanks for fixing up the code in any case, I'll take another crack at the sector options and see how far I get. I think you are right, it can be done with Min/Max values alone to keep from adding another set of props. I'll pull from the latest dev branch and make sure I'm not working on old code!

More hacking ahead!

Thanks and please never feel like you can't give me pointers, I pretty much learn from others examples and using the debugger these days so always really appreciate code feedback, helps me do a better job and possibly learn something 😄

Sandy