Zandronum Chat on our Discord Server Get the latest version: 3.1
Source Code

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0003996Doomseeker[All Projects] Cleanuppublic2022-05-04 04:142022-05-04 21:13
ReporterWubTheCaptain 
Assigned ToZalewa 
PrioritynormalSeveritytrivialReproducibilityalways
Statusneeds testingResolutionopen 
PlatformOSOS Version
Product Version1.3.2 
Target Version1.4Fixed in Version 
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 Informationhttps://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 (developer)
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 (developer)
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 (developer)
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 (developer)
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!

Issue Community Support
Only registered users can voice their support. Click here to register, or here to log in.
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






Questions or other issues? Contact Us.

Links


Copyright © 2000 - 2022 MantisBT Team
Powered by Mantis Bugtracker