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

chatty: init at 0.3.2 #122373

Merged
merged 2 commits into from Sep 1, 2021
Merged

chatty: init at 0.3.2 #122373

merged 2 commits into from Sep 1, 2021

Conversation

dotlambda
Copy link
Member

Motivation for this change

continuation of #94820
I decided against the wrapper approach as that makes it hard to override other things and chatty doesn't take long to build. If a wrapper is preferred though, I'll change it of course.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I still have to do extensive testing of all the plugins.

It would be nice if libpurple were separate from pidgin.

@Pacman99
Copy link
Contributor

Pacman99 commented May 9, 2021

I agree on dropping the wrapper approach. And chatty is moving towards putting plugins in tree, so its pretty unnecessary. It looks like purple-mm-sms will be moved into chatty itself soon.

Keep in mind many purple plugins don't actually work with chatty. I believe there is a hardcoded list of plugins the app supports.

For phosh/mobile usage, it might be good to include chatty with purple-mm-sms as a default package in the phosh module. Perhaps that can be saved for a future PR.

@dotlambda
Copy link
Member Author

For phosh/mobile usage, it might be good to include chatty with purple-mm-sms as a default package in the phosh module. Perhaps that can be saved for a future PR.

I was thinking of adding all supported plugins to the plugins list by default. If we add chatty to phosh's default packages, this would be especially nice because changing the plugins would require using an overlay rather than adding chatty.override { plugins = ...; } to systemPackages.

@Pacman99
Copy link
Contributor

Pacman99 commented May 9, 2021

Thats a good idea. Make sure not to include purple-matrix the native matrix support in chatty is much better.

Also another consideration, should an override just append to the default plugin list or should it overwrite the plugin list entirely?

@dotlambda
Copy link
Member Author

Make sure not to include purple-matrix the native matrix support in chatty is much better.

How do I use the native one?

Also another consideration, should an override just append to the default plugin list or should it overwrite the plugin list entirely?

I think overwriting it is the better choice, as people who override the package know which plugins exactly they want.

@dotlambda
Copy link
Member Author

Make sure not to include purple-matrix the native matrix support in chatty is much better.

How do I use the native one?

I had to do dconf write /sm/puri/Chatty/experimental-features true.

@dotlambda
Copy link
Member Author

Skimming through the code makes it seem like native matrix support is always used if experimental features are enabled, regarless of whether purple-matrix is installed. But I still have to confirm that.

@Pacman99
Copy link
Contributor

Pacman99 commented May 9, 2021

Skimming through the code makes it seem like native matrix support is always used if experimental features are enabled, regarless of whether purple-matrix is installed. But I still have to confirm that.

If thats true, then purple-matrix can just be included by default

I think overwriting it is the better choice, as people who override the package know which plugins exactly they want.

That makes sense to me

@dotlambda dotlambda changed the title chatty: init at 0.3.0 chatty: init at 0.3.1 May 28, 2021
@dotlambda
Copy link
Member Author

I can't seem to get my Matrix account to work. I put in my data and it shows an account was created. However, my account keeps showing as disconnected and even using --debug --verbose doesn't produce any related error messages.

@tomfitzhenry
Copy link
Contributor

FYI, #129230 is introducing purple-mm-sms.

@tomfitzhenry
Copy link
Contributor

I still have to do extensive testing of all the plugins.

It'd be great to get chatty in, even without all or any plugins (XMPP is included by default), to provide a Phosh-native chat client.

@dotlambda
Copy link
Member Author

@tomfitzhenry If you can confirm that this is working, we can definitely add it. I just couldn't get it to work with my Matrix account and didn't bother with making an XMPP one.

@tomfitzhenry
Copy link
Contributor

@tomfitzhenry If you can confirm that this is working, we can definitely add it. I just couldn't get it to work with my Matrix account and didn't bother with making an XMPP one.

Done. I was able to login into an XMPP room and communicate, with chatty.

@dotlambda dotlambda marked this pull request as ready for review July 5, 2021 11:15
@tomfitzhenry
Copy link
Contributor

I've been using this for 3 weeks now without issue. +1 to merging.

Copy link
Contributor

@Pacman99 Pacman99 left a comment

Choose a reason for hiding this comment

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

LGTM

@dotlambda
Copy link
Member Author

Who wants to step up to be chatty's maintainer? Since I can't test it, I shouldn't be.

@tomfitzhenry
Copy link
Contributor

Who wants to step up to be chatty's maintainer? Since I can't test it, I shouldn't be.

I am happy to be chatty's maintainer. I've been using this package for 7 weeks now, and am happy with the PR as-is. Please add me as the maintainer.

dotlambda and others added 2 commits September 1, 2021 09:27
Co-authored-by: Jordi Masip <jordi@masip.cat>
@dotlambda
Copy link
Member Author

Who wants to step up to be chatty's maintainer? Since I can't test it, I shouldn't be.

I am happy to be chatty's maintainer. I've been using this package for 7 weeks now, and am happy with the PR as-is. Please add me as the maintainer.

Thanks! I left myself in so I'm still pinged when chatty is modified.
Would you mind updating the package to 0.3.4 after this is merged?

@dotlambda dotlambda changed the title chatty: init at 0.3.1 chatty: init at 0.3.2 Sep 1, 2021
@dotlambda
Copy link
Member Author

Result of nixpkgs-review pr 122373 run on x86_64-linux 1

1 package built:
  • chatty

@ofborg ofborg bot requested a review from tomfitzhenry September 1, 2021 16:39
@tomfitzhenry
Copy link
Contributor

Would you mind updating the package to 0.3.4 after this is merged?

Yes, I will. I'm subscribed to https://source.puri.sm/Librem5/chatty/-/tags?format=atom too, for future releases.

@dotlambda dotlambda merged commit eb21f8d into NixOS:master Sep 1, 2021
@dotlambda dotlambda deleted the chatty-init branch September 1, 2021 22:20
@tomfitzhenry
Copy link
Contributor

Would you mind updating the package to 0.3.4 after this is merged?

Yes, I will.

#136527

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

Successfully merging this pull request may close these issues.

None yet

3 participants