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

Dynamically adapt step size for rate limit boxes #4943

Closed
wants to merge 1 commit into from

Conversation

HaraldNordgren
Copy link
Contributor

@HaraldNordgren HaraldNordgren commented Mar 12, 2016

This will dynamically adapt the step size of each of the four Rate Limit spinboxes to one order of magnitude below the value. When the rate limit is to 320 KiB/s, the step size will be 10, so that increasing it one step changes the value to 330 instead of 321. Increasing 1200 KiB/s will give 1300, and so on. Values 1-99 will still have step size 1.

The step size is calculated by taking log10 of the value every time it changes, rounding it down, subtracting by one, and inflating it again with pow. Special care is taken for single-digit values.

@@ -583,6 +587,12 @@ int options_imp::getProxyType() const
}
}

void options_imp::setLimitStep(QSpinBox *spinLimit, int value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be a pure function as

int options_imp::limitStep(int value) const
{
}

that means spinLimit->setSingleStep() (UI related logic) should be moved up to options_imp::adaptDownloadLimitStep and the like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, makes sense.

@@ -583,6 +587,11 @@ int options_imp::getProxyType() const
}
}

int options_imp::limitStep(int value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • coding style, dedicate a new line for {

  • move int pow10[7] = {1, 1, 10, 100, 1000, 10000, 100000}; to here.

    add const keyword to make it unmodifiable.
    remove 7, let the compiler count the elements (at compile-time)

    const int pow10[] = {1, 1, 10, 100, 1000, 10000, 100000};

That's all from me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Put the lookup table inside the function as a static const.

@Chocobo1
Copy link
Member

@HaraldNordgren
Wait...
take a look at this: http://doc.qt.io/qt-4.8/qabstractspinbox.html#accelerated-prop
IMO using built-in functionality is simpler, what do you think?

If you're going to use that, do it in options.ui (via Qt designer/creator)

@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Mar 12, 2016

@LordNyriox As I explained, rate limits between 1–99 KiB/s won't be affected by this.

I usually set an alternative download limit around 1000 KiB/s for when I want to download something and for example stream video at the same time. Depending on how close I am to my wireless router, I will usually tinker with it to find the sweet spot.

It's not useful to adjust the limits in steps 900, 901, 902, 903 KiB/s, etc. It hardly has any effect, so for higher numbers you are always better off just typing it.

So I wrote this because I saw a need for me personally. It's now easy to adjust the rate limit from 960 down to 940 KiB/s, gauge the performance, and then maybe down to 920 KiB/s. With this fix, the spinboxes scale so that using the up/down buttons always changes the limit by 1–10%.

The option is still there to set the limit to exactly 721 KiB/s if you would want that.

@Chocobo1 Yes, the accelerated property attacks the same problem. But this is a more satisfying solution in my opinions, except maybe for the fact that it adds more code.

I feel like most people would want to keep rate limits at even 10's or 100's. That is tricky using acceleration. You would often need a second action to adjust the number after it lands somewhere in-between. My solution keeps the limits as even 980, 990, 1000, 1100 KiB/s, etc. And there is no need to hold the button down and wait for the acceleration to set in.

@@ -1079,6 +1098,26 @@ void options_imp::enableApplyButton()
applyButton->setEnabled(true);
}

void options_imp::adaptDownloadLimitStep(int value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should implement only one function instead of four.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how would you go about doing that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may use QObject::sender() to get the widget which is needed to be tuned:

void options_imp::adaptStep(int value)
{
    static_cast<QSpinBox*>(sender())->setSingleStep(limitStep(value));
}

I guess @glassez meant exactly this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess @glassez meant exactly this.

Yes.

{
static const int pow10[] = { 1, 1, 10, 100, 1000, 10000, 10000 };
int log = static_cast<int>(log10(value));
return pow10[log];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a range check here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not only rely on what someone from the outside will do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added range check in adaptLimitStep by comparing against the max and min values from the spinBox, and throwing out_of_range exception if the value is outside of that.

@HaraldNordgren
Copy link
Contributor Author

So can we approve this?

@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Jun 12, 2016

@Chocobo1 Rebasing and build successful. Please approve this before it goes stale again :)

Copy link
Member

@glassez glassez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's completely useless. It's like "obsessive service". Why not enter the value you want explicitly? I do so always.
But if other developers will consider this feature useful, let it be.

Copy link
Contributor

@LordNyriox LordNyriox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

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

Successfully merging this pull request may close these issues.

5 participants