Skip to content

Conversation

cdanis
Copy link
Contributor

@cdanis cdanis commented Aug 29, 2025

Description (required)

Fixes #6411

What changes did you make and why?

For one of the OkHttpClient.Builder()s used inside the app, the one in OkHttpConnectionFactory.kt, there's a CommonHeaderRequestInterceptor class that adds User-Agent to outgoing requests.

The same isn't true of NetworkingModule.kt. So let's try adding it there.

Tests performed (required)

None, sorry.

I don't know Kotlin, do not have Android Studio installed, and really just want to see if this builds OK using the Github Actions flow (which I tried to run locally using act, but ran into emulator issues I couldn't quickly diagnose).

@cdanis cdanis force-pushed the user-agent-T400119 branch from 837db39 to 2697988 Compare August 29, 2025 17:13
@rohit9625
Copy link
Collaborator

Please don't force push @cdanis. Your import will only work if we make this class public:

private class CommonHeaderRequestInterceptor : Interceptor {

@rohit9625
Copy link
Collaborator

And yes, it fixes the problem. Thank you so much for pointing this out.

Screen_recording_20250829_224847.webm

Please make that class public and push your changes. Your PR will be merged tomorrow once approved :)

@cdanis
Copy link
Contributor Author

cdanis commented Aug 29, 2025

Please don't force push @cdanis. Your import will only work if we make this class public:

Apologies, I'm not used to force-pushes on PR branches in personal forks being a concern.

I've added a new commit making the referenced class public. Thanks so much!

@rohit9625
Copy link
Collaborator

rohit9625 commented Aug 29, 2025

Thanks @cdanis. Please link the correct issue #6411 in your description and comment on that issue so that I can assign you. Also, in Kotlin, we don't need to add public as classes are public by default; I forgot to mention that you might've just removed private. No worries it still works :)

@rohit9625 rohit9625 requested review from psh and rohit9625 and removed request for psh August 29, 2025 17:35
@rohit9625
Copy link
Collaborator

All good, just fix the import as the correct import should be:

import fr.free.nrw.commons.CommonHeaderRequestInterceptor

Also, please remove the redundant public keyword from the CommonHeaderRequestInterceptor class. Thanks :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the import with this import fr.free.nrw.commons.CommonHeaderRequestInterceptor.

@rohit9625
Copy link
Collaborator

Thanks for your quick help, @cdanis. It works great now and is ready to merge @nicolas-raoul :)

Screen_recording_20250829_234335.mp4

Copy link

✅ Generated APK variants!

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Chris for this very important fix, and Rohit for reviewing! Great collaboration. :-)

@nicolas-raoul nicolas-raoul merged commit 48e7eff into commons-app:main Aug 29, 2025
1 check passed
@RitikaPahwa4444
Copy link
Collaborator

Wow! Thanks a lot Chris and Rohit. I'll submit a patch today.

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

Successfully merging this pull request may close these issues.

Could not load nearby places
4 participants