MantisBT - Doomseeker |
| View Issue Details |
|
| ID | Project | Category | View Status | Date Submitted | Last Update |
| 0003996 | Doomseeker | [All Projects] Cleanup | public | 2022-05-04 04:14 | 2024-11-03 19:12 |
|
| Reporter | WubTheCaptain | |
| Assigned To | Zalewa | |
| Priority | normal | Severity | trivial | Reproducibility | always |
| Status | closed | Resolution | fixed | |
| Platform | | OS | | OS Version | |
| Product Version | 1.3.2 | |
| Target Version | 1.4.0 | Fixed in Version | 1.4.0 | |
|
| Summary | 0003996: Unused PRIORITY_CUSTOM in code; no remaining useful addFileSiteUrlWithPriority priority code |
| Description | Pol 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 Reproduce | git 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)
|
| Tags | No tags attached. |
| Relationships | |
| Attached Files | |
|
| 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 | bug_revision_view_page.php?bugnote_id=22212#r13596 |
| 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 |
|
Notes |
|
|
|
|
Also, any reason to need two variables DEFAULT_PRIORITY and PRIORITY_NORMAL with the same values (0)? |
|
|
|
(0022207)
|
|
Blzut3
|
|
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") |
|
|
|
|
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. |
|
|
|
|
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. |
|
|
|
|
|
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. |
|
|
|
(0022212)
|
|
Zalewa
|
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.
|
|
|
|
(0022213)
|
|
Zalewa
|
|
2022-05-04 19:55
|
|
|
|
|
(0022214)
|
|
Pol M
|
|
2022-05-04 20:54
|
|
|
|
|
(0022635)
|
|
Zalewa
|
|
2023-01-05 12:03
|
|
|
|
|
(0022690)
|
|
Zalewa
|
|
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. |
|