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

Add a few MMS fixes. #246

Closed
wants to merge 3 commits into from
Closed

Add a few MMS fixes. #246

wants to merge 3 commits into from

Conversation

KenMacD
Copy link

@KenMacD KenMacD commented Sep 7, 2022

In testing the mms branch I ran in to a few issues that these commits fix:

  • Only MMS messages were being loaded (looks like a new parameter is needed to load them all)
  • Images ended up with a lot of padding above and below them
  • MMS messages with only an image still showed the empty text line.

(Re: #190)

Both the getMMS and getSMS APIs now accept an 'all_messages'
parameter to load the other type at the same time.
Setting this eliminates the vertical padding when an image is
scaled to fit the conversation width.
@KenMacD
Copy link
Author

KenMacD commented Oct 19, 2022

Hi @michaelkourlas. I've been using a local build with the above for over a month now without issue. Any feedback on this PR. or MMS support in general? (I'm trying to decide if I want to set up push notifications)

@michaelkourlas
Copy link
Owner

For copyright and maintenance reasons I prefer to write my own code, so it's unlikely I'll end up merging this PR.

I don't have any immediate term plans to finish the MMS work, so if you'd like to add push messaging support for your own use in the meantime, please do!

@KenMacD
Copy link
Author

KenMacD commented Oct 20, 2022

Okay, thanks @michaelkourlas. Personally with these changes I'd consider the MMS work 'finished'. I'm not sure what else you had planned, but for me it works a treat.

As for the above changes, it's just a couple of simple fixes and I couldn't care less about the copyright or authorship, so feel free to copy/paste them to your own commit. I hereby multi-license this code under WTFPL, CC0, Unlicensed, and MIT-0.

@yulman19
Copy link

@KenMacD If @michaelkourlas doesn't want to do the pull, could you release a build?

@JohnMertz
Copy link

@KenMacD you don't need to declare a new license. It is Apache-2.0, which states, in part:

5. Submission of Contributions. Unless You explicitly state otherwise, any Contribution intentionally submitted for inclusion in the Work by You to the Licensor shall be under the terms and conditions of this License, without any additional terms or conditions.

Which is fine because this means you contribution is under the same copyright terms, notably:

2. Grant of Copyright License. Subject to the terms and conditions of this License, each Contributor hereby grants to You a perpetual, worldwide, non-exclusive, no-charge, royalty-free, irrevocable copyright license to reproduce, prepare Derivative Works of, publicly display, publicly perform, sublicense, and distribute the Work and such Derivative Works in Source or Object form.

So there is no real copyright conflict. I'm not sure that I understand the objection (nor would I want to speculate) given that other contributions have been merged in the past. Presumably the argument about maintainability could be valid if you are introducing a feature which could create a larger support burden or which is too big or complex to be brought under one individuals maintainership. The former would be an argument to adopt plugin functionality with separate issue tracking, while the later would be for co-maintainership, or even a community fork if absolutely necessary.

@KenMacD
Copy link
Author

KenMacD commented Oct 20, 2022

@KenMacD If @michaelkourlas doesn't want to do the pull, could you release a build?

@yulman19 To avoid the numerous issues that would cause, I was hoping to avoid creating one. That's partially why I asked for feedback here. If required I could generate one though.

@KenMacD you don't need to declare a new license. It is Apache-2.0...

@JohnMertz well, I'm not a lawyer and playing devils advocate, I'm not sure how much weight I'd put in me having implicitly entered in to a contract based on a license I haven't even read let alone signed anything. If I make my repo license state that contributors agree to pay me $1M for every PR opened I doubt I'll make any money. Maybe the conflict is around the relicensing mess is the problem (eg apparently Apache-2.0 isn't compatible with GPL-2.0)?

But as I said, I couldn't care less about authorship of these tiny fixes. Apache-2.0, copy/paste, whatever. I just needed mms support to pass the 'kid test', like others in #190. Heck feel free to copy/paste and then put in the changelog that the mms code was delayed because of me if you want.

@michaelkourlas
Copy link
Owner

Yes, it's not clear to me whether section 5 of the license is actually enforceable in practice. The right way to do things would be to obtain a signed contributor license agreement or copyright assignment, but I don't really want to go through that trouble.

With respect to maintenance, I tend to find it easier to maintain code that I wrote.

Beyond those two reasons, there's also the simple fact that as this is a hobby project, I prefer to write all of the code myself and have complete control and ownership over it. That's just part of the appeal for me.

It's true that I have accepted PRs in the past. However, they were very small, and even so I probably wouldn't accept them today.

@KenMacD
Copy link
Author

KenMacD commented Oct 20, 2022

It's true that I have accepted PRs in the past. However, they were very small, and even so I probably wouldn't accept them today.

Hopefully not getting too far off topic here, but might I kindly suggest putting that in a CONTRIBUTING.md to warn anyone who might be considering spending time on substantial changes?

@KenMacD KenMacD closed this Oct 20, 2022
@michaelkourlas
Copy link
Owner

michaelkourlas commented Oct 20, 2022

Yep, that's a good idea. I'll put something together.

@TurboLed
Copy link

TurboLed commented Nov 1, 2022

Hi
I'm waiting for a build of the MMS support, as I cannot succeed to build locally, I'm running into issues with securestoragelibrary as well as with Firebase.
If one of you can provide a release for the MMS branch or with instructions on how to build locally with Android Studio that would be much appreciated!
Lacking the app, I ran into crazy MMS fees by forwarding all the incoming MMS to my cell phone 👎

@xenotropic
Copy link

This seems tragic. Aren't we talking about four lines of code here? Is there any publicly-available apk of this fork? @KenMacD you previously said "If required I could generate [a build]", just wondering if that is still possibly on your to-do list.

@KenMacD
Copy link
Author

KenMacD commented Jan 11, 2023

@xenotropic Last month I reluctantly posted a debug build on my fork. It doesn't use the Google APIs for notifications (I don't have the key for that), so you'll have to weigh mms against quicker notifications.(I'd like to add UnifiedPush to it eventually).

@slevind
Copy link

slevind commented Jan 23, 2023

Hi @KenMacD , I have downloaded and installed the debug build. Thank you. I have noticed that I still get an empty line occasionally when there is only a picture attached to the message. Looking forward to the push notifications.

Is there also the ability to add an attach file to the message so a user can send MMS picture messages?

Cheers. Keep up the awesome work.

@xenotropic
Copy link

@KenMacD Your debug build is working pretty good for me, thank you. Do you know if the limiting factor for multi-recipient MMS ("group texts") is the VoiP.ms API -- i.e., that the information just isn't available? Or is the information there to compile group messages into threads, but no one has had the time to incorporate it into the app? Separately, you might also consider turning on issues on your fork.

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.

None yet

7 participants