Zandronum Chat on our Discord Server Get the latest version: 3.2
Source Code
My View | View Issues | Change Log | Roadmap | Doomseeker Issue Support Ranking | Rules | My Account
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0003996Doomseeker[All Projects] Cleanuppublic2022-05-04 04:142024-11-03 19:12
ReporterWubTheCaptain 
Assigned ToZalewa 
PrioritynormalSeveritytrivialReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version1.3.2 
Target Version1.4.0Fixed in Version1.4.0 
Summary0003996: Unused PRIORITY_CUSTOM in code; no remaining useful addFileSiteUrlWithPriority priority code
DescriptionPol M deleted code related to PRIORITY_CUSTOM, but kept the variable.
$ grep -r PRIORITY_CUSTOM src/
src/wadseeker/wadseeker.cpp:    const int PRIORITY_CUSTOM = 100;

In comparison, the other priorities are used, effectively there's no useful priority code remaining.
$ grep -r DEFAULT_PRIORITY src/
src/wadseeker/wwwseeker/wwwseeker.cpp:  const int DEFAULT_PRIORITY = 0;
src/wadseeker/wwwseeker/wwwseeker.cpp:  addFileSiteUrlWithPriority(filename, url, DEFAULT_PRIORITY);

$ grep -r PRIORITY_NORMAL src/
src/wadseeker/wadseeker.cpp:    const int PRIORITY_NORMAL = 0;
src/wadseeker/wadseeker.cpp:                                            d->wwwSeeker->addFileSiteUrlWithPriority(wad.fileName(), 
url, PRIORITY_NORMAL);
Steps To Reproducegit log -S PRIORITY_CUSTOM src/wadseeker/wadseeker.cpp
Additional Information'https://bitbucket.org/Doomseeker/doomseeker/commits/611982a70570b59e67d617aef8013075c380e8ab [^]'

$ grep -r "addFileSiteUrlWithPriority" .
./src/wadseeker/wadseeker.cpp:                                          d->wwwSeeker->addFileSiteUrlWithPriority(wad.fileName(), 
url, PRIORITY_NORMAL);
./src/wadseeker/wwwseeker/wwwseeker.h:  void addFileSiteUrlWithPriority(const QString &filename, 
const QUrl &url, int priority);
./src/wadseeker/wwwseeker/wwwseeker.cpp:        addFileSiteUrlWithPriority(filename, url, DEFAULT_PRIORITY);

./src/wadseeker/wwwseeker/wwwseeker.cpp:void WWWSeeker::addFileSiteUrlWithPriority(const QString &filename, 
const QUrl &url, int priority)


Attached Files

- Relationships

-  Notes
User avatar (0022206)
WubTheCaptain (reporter)
2022-05-04 04:15

 Also, any reason to need two variables DEFAULT_PRIORITY and PRIORITY_NORMAL with the same values (0)?
User avatar (0022207)
Blzut3 (administrator)
2022-05-04 04:24

 The reason there are two is they're in different scopes. PRIORITY_CUSTOM looks to be completely vestigial, and PRIORITY_NORMAL/DEFAULT_PRIORITY are probably a hold over from a former API. Honestly both should be removed and a default should be set in the prototype for WWWSeeker::addFileSiteUrlWithPriority. (i.e. "int priority=0")
User avatar (0022208)
WubTheCaptain (reporter)
2022-05-04 04:35
Quote from wadseeker.cpp
void Wadseeker::setupSitesUrls()
{
    // ...
        for (const auto &urlStr : d->seekParametersForCurrentSeek->customSiteUrls)
                incorporateUrl(true, urlStr);

        for (const QString &strSiteUrl : d->seekParametersForCurrentSeek->sitesUrls)
                incorporateUrl(false, strSiteUrl);
}

This code still prioritizes searching custom site URLs first (as before), so the priority parameter is vestigial yeah.
User avatar (0022209)
WubTheCaptain (reporter)
2022-05-04 05:29
Quote from Blzut3
Honestly both should be removed and a default should be set in the prototype for WWWSeeker::addFileSiteUrlWithPriority. (i.e. "int priority=0")

WWWSeeker::addFileSiteUrl also provides this API, with priority 0 (takes no parameter for priority). addFileSiteUrl is still used by WWWSeeker::parseAsHtml.
The new Wadseeker::setupSitesUrls code uses WWWSeeker::addFileSiteUrlWithPriority while setting the priority to 0 anyway.
User avatar (0022210)
WubTheCaptain (reporter)
2022-05-04 05:32

 And if you're interested, I'm not sure how truthful the API comments remain in src/wadseeker/wwwseeker/wwwseeker.h anymore. addFileSiteUrl and addFileSiteUrlWithPriority.
User avatar (0022212)
Zalewa (developer)
2022-05-04 19:04
edited on: 2022-05-04 19:32

 It's most probably a bug that PRIORITY_CUSTOM isn't used. Notice that 'isCustom' in incorporateUrl() isn't used either in the main if branch. This code was changed here:'https://bitbucket.org/Doomseeker/doomseeker/commits/611982a70570b59e67d617aef8013075c380e8ab#Lsrc/wadseeker/wadseeker.cppF511T516 [^]'
This is where PRIORITY_CUSTOM was lost.

We should restore PRIORITY_CUSTOM to its proper use.

As for DEFAULT_PRIORITY/PRIORITY_NORMAL duality. I can't precisely remember why this is duplicated. The original commit happened 8 years ago. Perhaps this happened by mistake. I think this may be corrected too.

However, using two different names to express the same value isn't always incorrect. As Blzut explained in one of the emails, the purpose of naming is for abstraction, self-documentation and as a means to express intent (this is also why sometimes simple conditional expressions, such as 'size == 0' or 'buffer == nullptr', may be encapsulated in a function called 'isEmpty()'). It also wouldn't be an error to declare 'DEFAULT = NORMAL = 0' in order to use DEFAULT for initialization, while use both DEFAULT and NORMAL for external use or comparison. If such constants are then used with their proper intent, it becomes trivial to later change the default value to something else, such as 'NORMAL = 0; DEFAULT = LOW = -1'. This is also useful for QModel columns if you intend to store extra, invisible data as a pointer in a specific column: COL_META = COL_NAME = 0.
User avatar (0022213)
Zalewa (developer)
2022-05-04 19:55

 Should be fixed:'https://bitbucket.org/Doomseeker/doomseeker/commits/2ad8b456c4e21219ad0809064ac52d35fe52b0f2 [^]'
User avatar (0022214)
Pol M (developer)
2022-05-04 20:54

 Reviewed :D
All good!
User avatar (0022635)
Zalewa (developer)
2023-01-05 12:03

 Beta package for Windows available at the beta auto-update channel and at:'https://devbuilds.drdteam.org/doomseeker/doomseeker-1.3.3~beta-230105-1140_windows.zip [^]'

Please test if Wadseeker correctly prioritizes the custom URLs now.
User avatar (0022690)
Zalewa (developer)
2023-01-14 00:06

 The priorities for the custom sites may still not work exactly as expected by the user because Wadseeker probes many websites at once. So, even though the custom site is the first one to be probed indeed (has the highest probing priority), other sites are also probed in parallel. Wadseeker doesn't currently wait for the priority site to finish, but instead immediately begins WADs downloads as soon as it sees a viable URL. This means that the WADs may begin downloading from the non-priority site even though they were also available on the priority site.

This issue, however, is out of the scope of this ticket, which was meant to clean-up the PRIORITY_CUSTOM and addFileSiteUrlWithPriority() in the code and to fix the priority not being assigned to the %WADNAME% URLs. I have checked all of that and all appears ok. The %WADNAME% custom URLs do get the high priority now.

Issue Community Support
This issue is already marked as resolved.
If you feel that is not the case, please reopen it and explain why.
Supporters: No one explicitly supports this issue yet.
Opponents: No one explicitly opposes this issue yet.

- Issue History
Date Modified Username Field Change
2022-05-04 04:14 WubTheCaptain New Issue
2022-05-04 04:14 WubTheCaptain Status new => assigned
2022-05-04 04:14 WubTheCaptain Assigned To => Pol M
2022-05-04 04:15 WubTheCaptain Note Added: 0022206
2022-05-04 04:21 WubTheCaptain Status assigned => new
2022-05-04 04:24 Blzut3 Note Added: 0022207
2022-05-04 04:35 WubTheCaptain Note Added: 0022208
2022-05-04 05:29 WubTheCaptain Note Added: 0022209
2022-05-04 05:32 WubTheCaptain Note Added: 0022210
2022-05-04 19:04 Zalewa Note Added: 0022212
2022-05-04 19:28 Zalewa Assigned To Pol M =>
2022-05-04 19:28 Zalewa Status new => confirmed
2022-05-04 19:32 Zalewa Note Edited: 0022212 View Revisions
2022-05-04 19:55 Zalewa Note Added: 0022213
2022-05-04 19:55 Zalewa Assigned To => Zalewa
2022-05-04 19:55 Zalewa Status confirmed => needs review
2022-05-04 20:54 Pol M Note Added: 0022214
2022-05-04 20:54 Pol M Status needs review => needs testing
2022-05-04 21:13 WubTheCaptain Target Version => 1.4.0
2023-01-05 12:03 Zalewa Note Added: 0022635
2023-01-14 00:06 Zalewa Note Added: 0022690
2023-01-14 00:06 Zalewa Status needs testing => resolved
2023-01-14 00:06 Zalewa Fixed in Version => 1.4.0
2023-01-14 00:06 Zalewa Resolution open => fixed
2024-11-03 19:12 Zalewa Status resolved => closed




Questions or other issues? Contact Us.

Links

Copyright © 2000 - 2025 MantisBT Team
Powered by Mantis Bugtracker