search

bgrabitmap / bgracontrols

🆗 BGRA Controls is a set of graphical UI elements that you can use with Lazarus LCL applications.
https://bgrabitmap.github.io/bgracontrols/
182 stars 30 forks source link

Changed ktSector Operation to compute divisions for sectors. #177

Closed sganz closed 3 months ago

sganz commented 3 months ago

Changed ktSector Operation to compute divisions for sectors. Removed SectorDivisions property. Some general clean up.

circular17 commented 3 months ago

Hi @sganz

From what I gather, the code you've used wasn't the latest as the check on FPC_FULLVERSION was reverted and some code moved. I'll fix that. the takeaway here is not to approve to quickly your changes and rather wait for someone else, me or Lainz to have quick look.

lainz commented 3 months ago

Hi. A good solution is to delete the branch and work in dev-bgracontrols directly. I think you have access.

circular17 commented 3 months ago

Delete which branch?

lainz commented 3 months ago

The branch on his/her github account.

sganz commented 3 months ago

@circular17 I messed up the commit, and likely went back to the version that had the wrong code for the compiler option, manual code update mistake, but I think I was on the current branch except for my handy work on this one.

@lainz yes, I think that is a good idea. I forked it so when I made github mistakes they would initially only mess with my repo until I got them sorted out 😢

I need to be more careful and just stop instead of trying to fix stuff up, which in my case always seems to make it worse.

Hopefully the code is good.

Sandy

circular17 commented 3 months ago

Hi Leandro,

The branch on his/her github account.

Ah indeed updating a fork is not always obvious to do.

Hi Sandy,

Yes, sometimes starting fresh is the simplest option. It can take time to change our habits.

I've reviewed the changes and added your beautiful test program. It all works fine and I've made slight adjustements so that changing the knob type doesn't reset the value:.

I've added default values for properties so that they are not written in the LFM file.

Thinking about it, the sector division doesn't seem necessary. The sector span is always equal to 1. So CalcSectorFromValue could simply round the value, or am I missing something? And CalcValueFromSector is in fact not necessary. What do you think?

Regards

sganz commented 3 months ago

I deleted my forked bgracontrols repo, hopefully that will be a better way to go. Let's see how that works out...

And yes, in looking at the CalSectorFromValue and CalcValueFromSector both seem like can be removed or cleaned up as sectorSpan will always be 1. I need to check to see if any of the range limit checks in CalcValueFromSector are really needed, as that may be the only thing, but have to take a look. Good catch, simpler is better! Will do some more coding tonight and see if all good, and then attempt to not botch up the git work :)

No problem changing anything in the tester program, glad it's useful, was helpful for me doing a lot of simple and repetitive testing.

Sandy

circular17 commented 3 months ago

Yes, it is practical to have a test program to play with the components!

I've reviewed the new changes, and they look perfect. The modifications are clear and easy to distinguish. Thanks for the efforts, Sandy!