Fix ETA for match-all share limits

[TurboTheTurtle]

Closes #24490.

Makes finished-torrent ETA respect the configured share-limit mode: Match Any reports the first enabled limit that can be reached, while Match All reports the time until the last enabled limit is satisfied.

[glassez]

IMO, it is still too much clutter. I believe entire method would look better as the following:

qlonglong TorrentImpl::eta() const
{
    if (isStopped())
        return MAX_ETA;

    const SpeedSampleAvg speedAverage = m_payloadRateMonitor.average();

    if (isFinished())
    {
        const qint64 ZERO_ETA = 0;

        const ShareLimits shareLimits = effectiveShareLimits();
        QList<qint64> etaList;

        if (shareLimits.ratioLimit >= 0)
        {
            qint64 realDL = totalDownload();
            if (realDL <= 0)
                realDL = wantedSize();

            const qreal uploadLimit = realDL * shareLimits.ratioLimit;
            const qint64 ratioEta = (speedAverage.upload > 0)
                    ? std::max<qint64>(((uploadLimit - totalUpload()) / speedAverage.upload), ZERO_ETA)
                    : MAX_ETA;
            etaList.append(ratioEta);
        }

        if (shareLimits.seedingTimeLimit >= 0)
        {
            const qint64 seedingTimeEta = std::max(
                    ((shareLimits.seedingTimeLimit * 60) - finishedTime()), ZERO_ETA);
            etaList.append(seedingTimeEta);
        }

        if (shareLimits.inactiveSeedingTimeLimit >= 0)
        {
            const qint64 inactiveSeedingTimeEta = std::max(
                    ((shareLimits.inactiveSeedingTimeLimit * 60) - timeSinceActivity()), ZERO_ETA);
            etaList.append(inactiveSeedingTimeEta);
        }

        if (etaList.isEmpty())
            return MAX_ETA;

        if (shareLimits.mode == ShareLimitsMode::MatchAny)
            return std::ranges::min(etaList);

        return std::ranges::max(etaList);
    }

    if (!speedAverage.download)
        return MAX_ETA;

    return (wantedSize() - completedSize()) / speedAverage.download;
}

[thalieht]

Setting the ratio to 0.0 in combination with any of the other limits always shows infinite ETA.
I can't test with any other ratio because i only have testing torrents.

[glassez]

Setting the ratio to 0.0 in combination with any of the other limits always shows infinite ETA. I can't test with any other ratio because i only have testing torrents.

How does it behave in master?

[thalieht]

How does it behave in master?

As expected, ignored/considered fulfilled.

[glassez]

Setting the ratio to 0.0 in combination with any of the other limits always shows infinite ETA. I can't test with any other ratio because i only have testing torrents.

I can't test/debug it right now. Maybe I'll be able to do it in a couple of days, if someone else doesn't figure it out sooner.

[glassez]

Does anyone have any idea why it returns MAX_ETA for all stopped torrents? What about the stopped finished ones? Especially if they are stopped due to reaching the share limit.

[TurboTheTurtle]

@glassez That behavior exists on 'master', and it does the same for the stopped finished ones, including stopped due to reaching the share limit. I tested it on the current 'master' and have been seeing it behave like that for a while. What should be the correct behavior?

[glassez]

That behavior that exists on 'master',

I didn't claim otherwise. I just came across it while reviewing this PR, and it seemed suspicious to me.

What should be the correct behavior?

I need to think about this carefully.

[TurboTheTurtle]

That makes sense. If you decide this should be displayed as “not applicable” rather than as an infinite/unknown ETA, I think N/A would be more consistent than -. qBittorrent already uses N/A in several GUI/WebUI places for unavailable or not-applicable values, while - seems to be used only in a few narrower placeholder cases.

I’d prefer to keep this PR focused on fixing the match-all share-limit ETA calculation, but I’d be happy to create a separate PR for the ETA display behavior if that’s the direction maintainers want.

[glassez]

I’d prefer to keep this PR focused on fixing the match-all share-limit ETA calculation

👍

[glassez]

If you decide this should be displayed as “not applicable” rather than as an infinite/unknown ETA

You probably didn't understand what exactly is bothering me.
IMO, there is no problem with infinite ETA in case of stopped unfinished torrents.
But stopped finished torrents could take into account whether they have reached the share limits or not. If so, the ETA should be zero, otherwise infinite.
Does it look reasonable?

[glassez]

I.e.:

const qint64 eta = (shareLimits.mode == ShareLimitsMode::MatchAny)
        ? std::ranges::min(etaList) : std::ranges::max(etaList);
if (!isStopped())
    return eta;

return (eta > ZERO_ETA) ? MAX_ETA : ZERO_ETA;

[TurboTheTurtle]

IMO, there is no problem with infinite ETA in case of stopped unfinished torrents. But stopped finished torrents could take into account whether they have reached the share limits or not. If so, the ETA should be zero, otherwise infinite. Does it look reasonable?

Got it. Yes, that seems like the correct behavior to have. I'll apply the patch

[glassez]

Got it. Yes, that seems like the correct behavior to have. I'll apply the patch

I'll apply the patch

Why are you "chasing horses" like that? Didn't we agree above that it's better not to mix this into this PR? It may still have objections from someone else.

[TurboTheTurtle]

Setting the ratio to 0.0 in combination with any of the other limits always shows infinite ETA. I can't test with any other ratio because i only have testing torrents.

Pushed a new fix. I tested the patched build with a throwaway profile through the WebUI API:

• Complete torrent, upload speed 0, ratio 0.0, seeding time limit 60.
• MatchAll: ETA returned finite, correct time, not infinite.
• MatchAny: ETA returned 0.

[glassez]

Pushed a new fix.

So what was the problem?

[TurboTheTurtle]

So what was the problem?

The problem was that the ratio ETA branch treated zero upload speed as MAX_ETA before checking whether the ratio target was already fulfilled.

For ratio limit 0.0, the upload target is 0, so a completed torrent with totalUpload() == 0 should contribute ETA 0. The previous version appended MAX_ETA whenever speedAverage.upload == 0, so in match-all mode it dominated the other finite limits and showed infinite ETA. The new version checks uploadLimit > totalUpload() first and only depends on upload speed when more upload is still needed.

[TurboTheTurtle]

Updated and manually verified on macOS with an isolated profile.

The stopped finished torrent that has not reached its ratio limit shows ∞, while the stopped finished torrent whose ratio limit is already fulfilled shows 0.

[V0latyle]

Tested with Windows CI 10985. All of these torrents are seeding, all of them have the same share limits from category. Appears to be working correctly.

Edit: I should note that when a finite ratio limit is set, any torrent that has not reached that ratio shows a blank ETA, but this is expected.

[thalieht]

Why force the stopped and finished torrents go through all those calculations below when we already know their ETA will be zero?
Also i imagine that if someone has many completed and stopped torrents, sorting by ETA will be impractical because the torrents which we actually care about to see their ETA will be somewhere in the middle of the list (completed at the beginning because now their ETA will be zero and stopped at the end because... infinity).

[TurboTheTurtle]

Stopped + finished does not necessarily mean ETA is zero. We still need to evaluate the effective share limits to distinguish fulfilled limits from unfulfilled ones. The current code computes the finished-torrent ETA first, then clamps stopped torrents to either 0 when the limits are fulfilled or MAX_ETA otherwise.

You’re right about the sorting consequence: ETA sorting uses the numeric ETA value, so fulfilled stopped/finished torrents will sort with 0, while stopped unfinished/unfulfilled torrents sort with MAX_ETA. If maintainers prefer stopped torrents to stay grouped differently when sorting by ETA, I think that would need a separate sorting/display policy change rather than changing this ETA calculation.

[thalieht]

Stopped + finished does not necessarily mean ETA is zero. We still need to evaluate the effective share limits to distinguish fulfilled limits from unfulfilled ones.

My bad.

[glassez]

Let's extract this change in its own PR as was decided previously.

[TurboTheTurtle]

Let's extract this change in its own PR as was decided previously.

Agreed. I’ll remove the stopped-finished ETA behavior from this PR and keep it focused on the match-all share-limit ETA fix. I'll follow up with that behavior in a separate PR.