yuya373 / emacs-slack

slack client for emacs
1.11k stars 117 forks source link

Slack already sends info about image without height width and bytes #532

Closed michal800106 closed 2 years ago

michal800106 commented 3 years ago

we should make optional fields (image_height, image_width and image_bytes) in slack-image-block-element because slack doesn't send it. I checked and with this change channels are properly read but I'm using emacs in terminal only so I couldn't check if images are loaded properly.

dyroffk commented 3 years ago

For what it's worth I've tested this solution and it resolves the channels failing to load I was experiencing and the image in question rendered successfully 2020-09-21-212855_322x63_scrot

rafauke commented 3 years ago

I can also confirm that it fixed the issue for me :+1:

jamesaimonetti commented 3 years ago

Confirming this fix works for me for problematic channels.

Konubinix commented 3 years ago

Same for me, I had this issue and the patch fixed it. Thanks @michal800106 for the fix and @yuya373 for the great emacs package :-).

Konubinix commented 3 years ago

I just updated emacs, rerun cask and fall on this issue once again. I fortunately remembered that this issue was patched here.

I resisted the temptation to fork the repository to have the patch, because this generally result in me not updating ever.

Then, I created a «hacky» command to patch the installed slack on the fly. https://github.com/Konubinix/Devel/commit/e16c3d38db8717477e3d658321d71398dd762c58

But then I wondered how do other people cope with patches not already merged upstream.

So I am asking @jamesaimonetti , @rafauke , @dyroffk how to keep the patch so that it follows possible updates of slack.el till it is merged upstream?

juboba commented 3 years ago

I just applied the changes from this patch and it seems to be working. Is this PR still being reviewed?

dyroffk commented 3 years ago

@yuya373 anything we can do to help to get this merged into master? We seem to have a number of people confirming it fixes the issue at hand

@Konubinix unfortunately I just forked it locally, made the changes, and checked upstream periodically for the fix to get merged. Thanks for the link to your patch function; I learned some things

alexiscott commented 3 years ago

Also working for me when I patch locally

skybert commented 3 years ago

@yuya373 anything we can do to help to get this merged into master? We seem to have a number of people confirming it fixes the issue at hand

Mhmm no answer after 2 months. I guess @yuya373 is busy with other projects (thanks for this amazing piece of software, btw). Two months without feedback on a 3 line PR seems to indicate the maintainer(s) are too busy elsewhere, and it's time to move this somewhere else IMHO. Last commit to master was 2020-08-30.

Konubinix commented 3 years ago

Torstein Krause Johansen @.***> writes:

@yuya373 anything we can do to help to get this merged into master? We seem to have a number of people confirming it fixes the issue at hand

Mhmm no answer after 2 months. I guess @yuya373 is busy with other projects (thanks for this amazing piece of software, btw). Two months without feedback on a 3 line PR seems to indicate the maintainer(s) are too busy elsewhere, and it's time to move this somewhere else IMHO. Last commit to master was 2020-08-30.

Do you mean find another maintainer?

-- Konubinix GPG Key : 7439106A Fingerprint: 5993 BE7A DA65 E2D9 06CE 5C36 75D2 3CED 7439 106A

skybert commented 3 years ago

Torstein Krause Johansen @.***> writes:

Mhmm no answer after 2 months. I guess @yuya373 is busy with other projects (thanks for this amazing piece of software, btw). Two months without feedback on a 3 line PR seems to indicate the maintainer(s) are too busy elsewhere, and it's time to move this somewhere else IMHO. Last commit to master was 2020-08-30.

Do you mean find another maintainer?

Yes, preferably adding this person as a co-maintainer/github repo collaborator to this, existing repo.

Konubinix commented 3 years ago

Now, new mpim are not taken into, most likely due to the fact it uses the old deprecated API. So indeed, this package needs some love. The code appears to already use a little bit the new API, but to completely.

The issue is. Who can take over?

dchenbecker commented 3 years ago

Assuming we get no reply from @yuya373 , can we publish to ELPA from a fork? I'm happy to help maintain things, but I'm not sure what the process would be. The simplest would seem to be Having Yuya add one or more of us to this repo as admins, but short of that what are our options?

skybert commented 3 years ago

@dchenbecker: I assume you cannot publish to ELPA using the same package identifier, you'd have to change the package name like slack2, slack-ng or similar.

As for the process, I don't have any experience with publishing package to ELPA, but wager you'd start off by forking this repo and follow the instructions on https://github.com/melpa/melpa/blob/master/README.md

yuya373 commented 2 years ago

I fixed height and width without noticing this PR in e332736fe6102a9b4c2cda1441d148765938f935 , Sorry.