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 the function to upload to tgraph #431

Closed
wants to merge 2 commits into from

Conversation

zxcvbnmzsedr
Copy link

Upload the image directly to tgraph without proxying through another site.

@zxcvbnmzsedr
Copy link
Author

No problem, I didn't consider that much; this way is the quickest and most straightforward solution to my current problem. I will make some adjustments to the code based on your advice.

Copy link
Owner

@Rongronggg9 Rongronggg9 left a comment

Choose a reason for hiding this comment

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

Personally, I do not object to the feature, as long as it is opt-in (it does be in the current revision). But it is more like a "dirty hack" as far as I can see now, so more polishing is needed before it can be merged.

requirements.txt Outdated
@@ -38,3 +38,4 @@ CJKwrap==2.2
typing-extensions==4.10.0
uvloop==0.19.0; sys_platform!='win32' and sys_platform!='cygwin' and sys_platform!='cli'
isal==1.6.0; platform_machine=='x86_64' or platform_machine=='AMD64' or platform_machine=='aarch64'
requests==2.26.0
Copy link
Owner

Choose a reason for hiding this comment

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

RSStT is asynchronous, using synchronous HTTP lib will make the bot stuck.
Use the web wrapper (in src/web/req.py) instead.

src/env.py Outdated
@@ -154,6 +154,7 @@ def __list_parser(var: Optional[str]) -> list[str]:
API_ID: Final = int(os.environ['API_ID']) if os.environ.get('API_ID') else None
API_HASH: Final = os.environ.get('API_HASH')
TOKEN: Final = os.environ.get('TOKEN')
TELEGRAPH_IMG_UPLOAD: Final = os.environ['TELEGRAPH_IMG_UPLOAD'] if os.environ.get('TELEGRAPH_IMG_UPLOAD') else False
Copy link
Owner

Choose a reason for hiding this comment

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

Document it in docs/advanced-settings.md, docker-compose.yml.sample and .env.smaple.
And use __bool_parser() above.

@@ -269,7 +272,6 @@ async def generate_page(self):
elif tag.name not in TELEGRAPH_ALLOWED_TAGS:
tag.replaceWithChildren() # remove disallowed tags
continue

Copy link
Owner

Choose a reason for hiding this comment

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

Unneeded reformatting.

@@ -278,7 +280,6 @@ async def generate_page(self):
alt = tag.get('alt')
tag.replaceWith(alt) if alt else tag.decompose() # drop emoticon
continue

Copy link
Owner

Choose a reason for hiding this comment

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

Ditto.

attr_content = telegraph_file_upload(attr_content)
else:
attr_content = construct_weserv_url(attr_content)
logger.warning(f'Processed img: {attr_content}')
Copy link
Owner

Choose a reason for hiding this comment

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

Why warning?

upload_response = requests.post(url, files={'file': ('file', image_data)})
telegraph_url = json.loads(upload_response.content)
telegraph_url = telegraph_url[0]['src']
telegraph_url = f'https://telegra.ph{telegraph_url}'
Copy link
Owner

Choose a reason for hiding this comment

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

The domain of Telegraph should be a const variable outside the function.

Comment on lines +391 to +393
telegraph_url = json.loads(upload_response.content)
telegraph_url = telegraph_url[0]['src']
telegraph_url = f'https://telegra.ph{telegraph_url}'
Copy link
Owner

Choose a reason for hiding this comment

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

All three lines can be inlined into one line.

error, txt-file can not be processed
'''
response = requests.get(file_url)
if response.status_code == 200:
Copy link
Owner

Choose a reason for hiding this comment

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

Only using a condition is not sufficient. Exception handling (fallback) is needed.

@@ -1,7 +1,10 @@
from __future__ import annotations

import json
Copy link
Owner

Choose a reason for hiding this comment

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

Adding import should be done on hand. Do not rely on your IDE to do it.
Read the import blocks twice to see where to add import.

from typing import Union, Optional
from typing_extensions import Final
from collections.abc import Awaitable
import requests
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto.

Comment on lines +248 to +250
async def _post(url: str, data: Optional[dict], resp_callback: Callable, timeout: Optional[float] = sentinel,
semaphore: Union[bool, asyncio.Semaphore] = None, headers: Optional[dict] = None,
read_bufsize: int = DEFAULT_READ_BUFFER_SIZE, read_until_eof: bool = True) -> WebResponse:

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns Note

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
v6_rr_set = None
try:
v6_rr_set = (await asyncio.wait_for(resolve(host, 'AAAA', lifetime=1), 1.1)).rrset if env.IPV6_PRIOR else None
except DNSException:

Check notice

Code scanning / CodeQL

Empty except Note

'except' clause does nothing but pass and there is no explanatory comment.
v6_rr_set = (await asyncio.wait_for(resolve(host, 'AAAA', lifetime=1), 1.1)).rrset if env.IPV6_PRIOR else None
except DNSException:
pass
except asyncio.TimeoutError:

Check notice

Code scanning / CodeQL

Empty except Note

'except' clause does nothing but pass and there is no explanatory comment.
@Rongronggg9
Copy link
Owner

Oh, I thought of a much more elegant solution for your use case just now.
Upload media to Telegraph via https://github.com/Rongronggg9/rsstt-img-relay so that it can be enabled for all users (opt-out).

For now, I am going to close your PR. I will implement the feature I mentioned above soon in the following weeks - you don't need to do anything further but just looking forward to it!

@zxcvbnmzsedr
Copy link
Author

Wow, I'm really looking forward to it.

@Rongronggg9
Copy link
Owner

I will implement the feature I mentioned above soon in the following weeks

Such a feature has been implemented in Rongronggg9/rsstt-img-relay@7502104. E.g., /upload_graph/https://example.com/example.jpg.

The next step is to implement the corresponding feature in RSStT to take advantage of it.

@zxcvbnmzsedr
Copy link
Author

I found that tgraph has certain restrictions on uploaded media files, such as requiring the dimensions to be within 5000*4000, the file size to be less than 10MB, and not accepting webp images. Uploading images that do not meet these requirements will result in an error message "Image dimensions invalid". Would handling this in JS be very cumbersome?
@Rongronggg9

@Rongronggg9
Copy link
Owner

Rongronggg9 commented Mar 20, 2024

Would handling this in JS be very cumbersome?

No need to actually "handle" it. All we need to do is to invoke https://images.weserv.nl/ on demand.

@Rongronggg9
Copy link
Owner

All we need to do is to invoke images.weserv.nl on demand.

Done in Rongronggg9/rsstt-img-relay@7115976

@Rongronggg9
Copy link
Owner

file size to be less than 10MB

The limit should be 5MiB.

Image dimensions invalid

Reconsider the case, we'd better let it fail in order not to resize long images (words within them will become invisible after resizing).
If an image is not too long, it is very unlikely to hit the dimension limit before hitting the size limit. In other words, we should filter out the "Image dimensions invalid" error message, preventing it from causing images to be resized.

Rongronggg9 added a commit that referenced this pull request Mar 28, 2024
See also #431

Signed-off-by: Rongrong <i@rong.moe>
Rongronggg9 added a commit that referenced this pull request Mar 28, 2024
See also #431

Signed-off-by: Rongrong <i@rong.moe>
@Rongronggg9
Copy link
Owner

All done 🎉

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.

2 participants