8000 Feature: Add option to attach image from Camera by shamim-emon · Pull Request #9074 · thunderbird/thunderbird-android · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Feature: Add option to attach image from Camera #9074

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 29, 2025
Merged

Feature: Add option to attach image from Camera #9074

merged 2 commits into from
Apr 29, 2025

Conversation

shamim-emon
Copy link
Contributor

@kewisch
Copy link
Member
kewisch commented Apr 23, 2025

Quick drive by comment - I believe we shouldn't be adding the strings to the translations, this is taken care of by weblate. I'll let @rafaeltonholo take a closer look at the rest.

rafaeltonholo
rafaeltonholo reviewed Apr 23, 2025
View reviewed changes
Copy link
Member
@rafaeltonholo rafaeltonholo left a comment

Choose a reason for hiding this comment

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

Hi @shamim-emon,

Thank you for your contribution! I have left a few comments, some of which require further discussion.

Additionally, I kindly ask you to revert all values-{lang}.xml files introduced in this pull request and retain only the values.xml file. Whenever a new string is added to the application, we should first include only the new strings using American English in values.xml. After this is merged, all other language translations should be added through our Weblate project.

For more information, please refer to Translations > Adding a string.

Thanks!

@shamim-emon shamim-emon force-pushed the fix-issue-7373 branch 2 times, most recently from 8e06bc4 to f220bc1 Compare April 25, 2025 08:45
@shamim-emon
Copy link
Contributor Author

Quick drive by comment - I believe we shouldn't be adding the strings to the translations, this is taken care of by weblate. I'll let @rafaeltonholo take a closer look at the rest.

@kewisch Removed added strings to the translations.

@shamim-emon shamim-emon force-pushed the fix-issue-7373 branch 2 times, most recently from cbb46ab to 3da906d Compare April 25, 2025 11:47
rafaeltonholo
rafaeltonholo requested changes Apr 28, 2025
View reviewed changes
Copy link
Member
@rafaeltonholo rafaeltonholo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @shamim-emon,

Thank you for addressing the other comments! I appreciate your suggestion to move the logic to a dedicated class. However, I have some concerns about creating the module :feature:camera.

According to the Project Structure, a Feature Module should be an independent unit that encapsulates distinct user-facing features. In this context, a camera feature module should offer significant user-facing capabilities, such as camera filters, video clipping, or post-image processing. I don't see these requirements as applicable to our app.

The current code seems to align better with the description of the core module, which mentions "essential utilities and base classes used across the entire project."

Additionally, I would advise against creating a new module for this, as it likely will consist of only a few files.

I suggest placing them in the :core:android:common module, where I believe they would be a better fit.

I would suggest the following structure, inside the :core:android:common module:

  • net.thunderbird.core.android.common.camera.CameraCaptureHandler (public api)
  • net.thunderbird.core.android.common.camera.provider.CaptureImageFileProvider (internal to the module)
  • net.thunderbird.core.android.common.camera.io.CaptureImageFileWriter (internal to the module)

Additionally, you will also need to create a CameraKoinModule.kt, adding the CameraCaptureHandler to the Koin dependency graph. Something like:

// net.thunderbird.core.android.common.camera.CameraKoinModule.kt
internal val cameraModule = module {
    single { CaptureImageFileWriter(context = get()) }
    single<CameraCaptureHandler> {
        DefaultCameraCaptureHandler(
            captureImageFileWriter = get(),
        )
    }
}

// and expose it to the other modules via `app.k9mail.core.android.common.CoreCommonAndroidModule.kt`

val coreCommonAndroidModule: Module = module {
    includes(coreCommonModule)

    includes(contactModule)

    // new Koin module
    includes(cameraModule)
}

Let me know your thoughts on this.

@shamim-emon
Copy link
Contributor Author
shamim-emon commented Apr 28, 2025 •
edited
Loading

Hi @shamim-emon,

Thank you for addressing the other comments! I appreciate your suggestion to move the logic to a dedicated class. However, I have some concerns about creating the module :feature:camera.

According to the Project Structure, a Feature Module should be an independent unit that encapsulates distinct user-facing features. In this context, a camera feature module should offer significant user-facing capabilities, such as camera filters, video clipping, or post-image processing. I don't see these requirements as applicable to our app.

The current code seems to align better with the description of the core module, which mentions "essential utilities and base classes used across the entire project."

Additionally, I would advise against creating a new module for this, as it likely will consist of only a few files.

I suggest placing them in the :core:android:common module, where I believe they would be a better fit.

I would suggest the following structure, inside the :core:android:common module:

  • net.thunderbird.core.android.common.camera.CameraCaptureHandler (public api)
  • net.thunderbird.core.android.common.camera.provider.CaptureImageFileProvider (internal to the module)
  • net.thunderbird.core.android.common.camera.io.CaptureImageFileWriter (internal to the module)

Additionally, you will also need to create a CameraKoinModule.kt, adding the CameraCaptureHandler to the Koin dependency graph. Something like:

// net.thunderbird.core.android.common.camera.CameraKoinModule.kt
internal val cameraModule = module {
    single { CaptureImageFileWriter(context = get()) }
    single<CameraCaptureHandler> {
        DefaultCameraCaptureHandler(
            captureImageFileWriter = get(),
        )
    }
}

// and expose it to the other modules via `app.k9mail.core.android.common.CoreCommonAndroidModule.kt`

val coreCommonAndroidModule: Module = module {
    includes(coreCommonModule)

    includes(contactModule)

    // new Koin module
    includes(cameraModule)
}

Let me know your thoughts on this.

@rafaeltonholo Done

rafaeltonholo
rafaeltonholo approved these changes Apr 29, 2025
View reviewed changes
Copy link
Member
@rafaeltonholo rafaeltonholo left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for your contribution to the project!

@rafaeltonholo rafaeltonholo merged commit 8c097d8 into thunderbird:main Apr 29, 2025
3 checks passed
@thunderbird-botmobile thunderbird-botmobile bot added this to the Thunderbird 11 milestone Apr 29, 2025
@shamim-emon shamim-emon deleted the fix-issue-7373 branch April 29, 2025 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaeltonholo rafaeltonholo rafaeltonholo approved these changes

@kewisch kewisch Awaiting requested review from kewisch kewisch is a code owner

@wmontwe wmontwe Awaiting requested review from wmontwe wmontwe is a code owner

@asoucar asoucar Awaiting requested review from asoucar asoucar is a code owner

Assignees

@rafaeltonholo rafaeltonholo

Labels
None yet
Projects
None yet
Milestone
Thunderbird 11
Development

Successfully merging this pull request may close these issues.

Option to attach from camera
3 participants
@shamim-emon @kewisch @rafaeltonholo
0