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

Card image loading: Fallback on 404 #3367

Merged
merged 14 commits into from Aug 22, 2018

Conversation

@andrewzwicky
Copy link
Contributor

@andrewzwicky andrewzwicky commented Aug 12, 2018

Related Ticket(s)

Short roundup of the initial problem

See #3363

What will change with this Pull Request?

  • Relative to #3363, the work has been updated to no longer use separately managed indices, and instead uses lists and takeFirst() to empty them.

  • Method for substituting information into Urls now gives more feedback about which items are failing, and will also return empty string instead of leaving empty strings inside the return Url.

Screenshots

andrewzwicky added 11 commits Aug 11, 2018
Reformatting files to be in line with style guidelines.
This change removes set and Url indices in favor
of check for empty lists and removing items from them
instead.
If transformUrl is called with a template, and the card/set
is missing something required by that template, it will now
return an empty string, instead of the template with an empty
string substituted in.
Adding PictureLoader: to the front of each debug message
from this file, so that it can be more easily filtered out
by grep in the log of a running application.
@tooomm tooomm changed the title [WIP] Fallback on 404 [WIP] Card image loading: Fallback on 404 Aug 12, 2018
@andrewzwicky
Copy link
Contributor Author

@andrewzwicky andrewzwicky commented Aug 12, 2018

I'm still planning to do more targeted testing to make sure things work under all conditions, but early feedback is always helpful.

Copy link
Contributor

@ctrlaltca ctrlaltca left a comment

The PR looks simpler and better now!
There are still minor things that could be reworked, but I'm fine to merge it also as-is.

}
}

if (!currentSetUrls.isEmpty()) {

This comment has been minimized.

@ctrlaltca

ctrlaltca Aug 16, 2018
Contributor

This few lines are basically an initial call to nextUrl()

This comment has been minimized.

@andrewzwicky

andrewzwicky Aug 21, 2018
Author Contributor

good catch, I'll update this as well.

if (cardBeingDownloaded.nextSet()) {
qDebug() << "Picture NOT found, download failed, moving to next set (newset: "
<< cardBeingDownloaded.getSetName() << " card: " << cardBeingDownloaded.getCard()->getName() << ")";
if (cardBeingDownloaded.nextUrl() || cardBeingDownloaded.nextSet()) {

This comment has been minimized.

@ctrlaltca

ctrlaltca Aug 16, 2018
Contributor

Note to future myself: this works because the || operator is short-circuiting: if the first operand is true, the second operand is not evaluated.

}
}

foreach (QString urlTemplate, urlTemplates) {

This comment has been minimized.

@ctrlaltca

ctrlaltca Aug 16, 2018
Contributor

The urlTemplates list contains a local copy of the unmodified picUrl strings from settingCache; I don't see it being useful in the present since we could just fetch the data from settingsCache here. Maybe you ahve an idea of using it to filter/modify the data coming from settingsCache?

This comment has been minimized.

@andrewzwicky

andrewzwicky Aug 16, 2018
Author Contributor

Yes, I am planning on updating the settingCache to contain a list of Urls instead of just two. This is in line with what #2479 was proposing. It will just make things easier to add in the future.

This comment has been minimized.

@ZeldaZach

ZeldaZach Aug 22, 2018
Member

My goal was to remove foreach from the codebase. Will get back to fixing this at some point in the future

This comment has been minimized.

@Daenyth

Daenyth Aug 22, 2018
Member

@ZeldaZach in favor of using map and fold functions? Nothing seems really terrible with foreach

This comment has been minimized.

@andrewzwicky

andrewzwicky Aug 22, 2018
Author Contributor

@ZeldaZach I'm newish to C++, what's the problem / prefered alternative for foreach?

@andrewzwicky
Copy link
Contributor Author

@andrewzwicky andrewzwicky commented Aug 16, 2018

I can clean up those few things and add some comments. It probably won't get done until mid next-week.

This removes some redundant code that is better replaced with a call
to nextUrl, in case the code needed to populate the nextUrl changes
significantly.
Refactor transformUrl to do everything in a single loop instead
of two almost identical loops.  set information is populated if
present, but is added with empty strings if absent.
@andrewzwicky
Copy link
Contributor Author

@andrewzwicky andrewzwicky commented Aug 21, 2018

If you're good with that, I'm satisfied with it.

@ctrlaltca ctrlaltca merged commit ed01752 into Cockatrice:master Aug 22, 2018
3 checks passed
3 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ctrlaltca
Copy link
Contributor

@ctrlaltca ctrlaltca commented Aug 22, 2018

Merged, thank you a lot for the code and your patience!

@tooomm tooomm changed the title [WIP] Card image loading: Fallback on 404 Card image loading: Fallback on 404 Aug 22, 2018
@tooomm
Copy link
Contributor

@tooomm tooomm commented Aug 22, 2018

Thanks for the feature! :)

@andrewzwicky please remove the [WIP] label from the PR title when you're done next time, please, and @ctrlaltca check such things before merging.
Actually there are GitHub Apps (Probot) out there that track that and prominently show it next to the other checks on each PR.

Otherwise it ends up in our commit list and is a bit confusing:
https://github.com/Cockatrice/Cockatrice/commits/master

I changed it now, but that won't change the history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.