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

[Console] Rename some methods related to redraw frequency #33732

Open
wants to merge 5 commits into
base: 4.4
from

Conversation

@javiereguiluz
Copy link
Member

commented Sep 27, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

In #26339 we added preventRedrawFasterThan() and forceRedrawSlowerThan(). While merging the docs for them (symfony/symfony-docs#12364) I thought that the method names are a bit hard to understand.

In this PR I propose a renaming for your consideration. Thanks!

In the following example, we want to update the progress bar every 100 iterations, but not faster than 100ms or slower than 200ms.

Before

$progressBar = new ProgressBar($output, 50000);
$progressBar->start();

$progressBar->setRedrawFrequency(100);
$progressBar->preventRedrawFasterThan(0.1);
$progressBar->forceRedrawSlowerThan(0.2);

After

$progressBar = new ProgressBar($output, 50000);
$progressBar->start();

$progressBar->setRedrawFrequency(100);
$progressBar->maxRefreshInterval(0.1);
$progressBar->minRefreshInterval(0.2);
@javiereguiluz javiereguiluz added this to the next milestone Sep 27, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

We considered these min/max names while working on this and decided they were more confusing than what we have now.
I have a really hard time understanding what "max" and "min" led to, in term of behavior.
While the current name make it crystal clear.

-0 on my side.

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

I have a mix feeling about this. I don't find the current name very clear, but I'm not sure the proposed one are way better.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

what about the actual property names? min/maxSecondsBetweenRedraws

that seems more clear compared to preventRedrawFasterThan/forceRedrawSlowerThan, as it conveys the technical behavior.

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2019

Actually, talking about "frames per second" or "refresh rate" is much more common than "frame duration" or "refresh interval". So, why don't we rename these methods as one of the following alternatives?

$progressBar->setRedrawFrequency(100);
$progressBar->minRefreshesPerSecond(5);
$progressBar->maxRefreshesPerSecond(10);

$progressBar->setRedrawFrequency(100);
$progressBar->minUpdatesPerSecond(5);
$progressBar->maxUpdatesPerSecond(10);

$progressBar->setRedrawFrequency(100);
$progressBar->minRedrawsPerSecond(5);
$progressBar->maxRedrawsPerSecond(10);
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

My opinion: maxRefreshInterval/minRefreshInterval are better :)

@Pierstoval

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

I'm in for minRedrawsPerSecond and maxRedrawsPerSecond, and I'd also remove setRedrawFrequency for setRedrawsPerSecond.

If no consensus can be found, minRedrawInterval and maxRedrawInterval are nice too.

And I give a lower score to maxRefreshInterval/minRefreshInterval, but it's still a good idea. It's just that "refresh" doesn't really exposes the real behavior in the end. Questions like: "what does a refresh do?" "it redraws the screen", "well, call it redraw then". To me, refresh is like service or manager: it's not clear enough, even if people with a lot of experience instantly understand it.

@jaikdean

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

minRedrawsPerSecond/maxRedrawsPerSecond are the easiest to understand for me. I expect this to vary a lot between people depending on how they picture the concept in their head, and probably their native language(s).

I can decipher them, but maxRefreshInterval(0.1) and minRefreshInterval(0.2) are inherently confusing to me. My first thought is “why is the max lower than the min?”. The method names feel the wrong way round, as they're mixing concepts of FPS vs frame duration.

@andrewmy

This comment has been minimized.

Copy link

commented Sep 30, 2019

First I'd like to note that frequency means a number of times per some time interval. E.g. a frequency of 100 Hz means 100 times per second.

To denote a number of iterations between actions I'd choose regularity.

Now that the word frequency is freed, we can use it to answer the question asked initially:

$progressBar->setRedrawRegularity(100);
$progressBar->minRedrawFrequency(5);
$progressBar->maxRedrawFrequency(10);

If we choose to stick to time units rather than frequency, I support min/maxRedrawInterval but not inverting the meaning.

// interval cannot be less than 0.1s => max freq of 10 Hz
$progressBar->minRedrawInterval(0.1);

// interval cannot be more than 0.2s => min freq of 5 Hz
$progressBar->maxRedrawInterval(0.2);
@TimoBakx

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

Starting the methods with just min and max feels a bit weird to me. From just the name, it's unclear if it's a setter or a getter or something else entirely (the original names didn't have this issue, though). Wouldn't it make more sense to use a setMin and setMax instead?

$progressBar->setRedrawFrequency(100);
$progressBar->setMaxRefreshInterval(0.1);
$progressBar->setMinRefreshInterval(0.2);
@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

Another proposition:

public function setRedrawInterval(float $minPerSec, float $maxPerSec = 0.1);
@cjunge-work

This comment has been minimized.

Copy link

commented Sep 30, 2019

I think @andrewmy's comment is important:

First I'd like to note that frequency means a number of times per some time interval. E.g. a frequency of 100 Hz means 100 times per second.

Frequency in the current usage is misleading. As the top description says, 100 is the number of iterations, which is not the definition of frequency. I'm not sold on regularity though, seems a bit vague IMHO. Using frequency to define the redraw makes sense too.

Copy link
Member

left a comment

Reading the discussion, I changed my mind and I'm 👍

The new wording is perfectly accurate on the technical level. Making it "boring" is a quality here.

I would not like having to define a frequency. 0.2 (redraw per seconds) is way harder to reason about than 5 (seconds between redraws).

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

Don't get mad at me 🙈 but ... now I'm the one who doesn't like my original proposal. maxRefreshInterval(0.1) may be easier to understand than preventRedrawFasterThan(0.1) ... but still requires to be explained (what's 0.1? What's an "interval"?).

The only proposal that I find self-explanatory is this one:

$progressBar->minRedrawsPerSecond(5);
$progressBar->maxRedrawsPerSecond(10);

Also, we already have a setRedrawFrequency() method, so "redraw" is a common word in this context. What do you think?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

The existing method that contains the word "frequency" accepts... an interval as argument.

I'm formally 👎 to use that term if that can help move forward :)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

@javiereguiluz please update the PR then... :)

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

@ostrolucky could you please help me with the tests? I'm having lots of problems trying to understand how things work.

This code for example is easy to understand:

$bar = new ProgressBar($output = $this->getOutputStream(), 100);
$bar->setRedrawFrequency(1);
$bar->minRedrawsPerSecond(5);
$bar->maxRedrawsPerSecond(10);
// ...
sleep(1);

The code sleeps for 1 second ... so we should have at least 5 redraws. Sadly, this is not how things work at all at the moment. Thanks for your help!

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

Naming things is hard 😓 So, after talking with Nicolas, I've renamed this again following his suggestion (I think that the new name is equally self-explanatory, but even easier to understand and avoids problems like the division by zero):

minSecondsBetweenRedraws(float $seconds)
maxSecondsBetweenRedraws(float $seconds)
Copy link
Member

left a comment

(with a minor comment)

$this->generateOutput(' 4 [---->-----------------------]'),
$this->generateOutput(' 3 [--->------------------------]').
$this->generateOutput(' 4 [---->-----------------------]').
$this->generateOutput(' 5 [----->----------------------]'),

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 4, 2019

Member

why do these tests need to change? same above

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Oct 4, 2019

Contributor

It took me a while to notice the problem. @javiereguiluz, you flipped (min|max)SecondsBetweenRedraws. This test case tests throttling, it needs to use minSecondsBetweenRedraws. To avoid this kind of bugs in future, please use IDE rename feature.

$this->generateOutput(' 4 [---->-----------------------]'),
$this->generateOutput(' 3 [--->------------------------]').
$this->generateOutput(' 4 [---->-----------------------]').
$this->generateOutput(' 5 [----->----------------------]'),

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Oct 4, 2019

Contributor

It took me a while to notice the problem. @javiereguiluz, you flipped (min|max)SecondsBetweenRedraws. This test case tests throttling, it needs to use minSecondsBetweenRedraws. To avoid this kind of bugs in future, please use IDE rename feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.