Added image editor#3106
Conversation
Adds image editor to the image send flow. Resolves issue krille-chan#2218 and krille-chan#3048
|
Have you used an LLM to write the code? |
|
I used an AI twice, once for a problem in the code I couldn't get to run, and I had some git problems where I worked with AI to fix it because I accidentally messed up git on my machine. |
Contribution Guidelines rule 2 says:
So I assume you haven't read them |
|
I am sorry if that is a problem for you. I read your guidelines, but I didn't thought that will be a problem. I carefully tested and verified the code and wrote like 99% by myself. If you have some problem with the code itself that's fine and I am willing to improve it. If you don't have a problem with the code itself but the process of making it (because I got a little bit help from ai in the process) and therefore don't approve a feature many people miss, I am sorry but I cannot help you and the project then. I think ai generated slop is shit and 100% vibe coded projects and pull requests which aren't even tested are shit. But constructive use of ai in software development is not wrong in my opinion. Edit: fixed typo |
|
I agree, this should not classify as a violation of "Contribution Guidelines rule 2" as the code was in fact NOT llm-generated, it was just used as a tool for debugging an issue. An image editor sounds like a great and well needed addition. |
|
@Tobias-Auer sounds reasonable and sorry for my very harsh answer. Just was because of the frustration that i have to deal with so many AI slop contributions these days. I'll review your PR asap |
|
I really hope you'll all agree at last and this can be added – because a basic image editor would be such an outstanding feature that would bring FluffyChat on par with other messengers out there. All the other Matrix clients lack this feature, so this would really be a unique feature. However, thanks for implementing this @Tobias-Auer and thanks for reviewing it @krille-chan in advance (I can understand your attitude towards AI stuff, although it sometimes actually does help and makes ones life easier when writing code – when really writing code. But yes, we should never-ever use AI code, not a single snippet, that we don't fully and completely understand). |
krille-chan
left a comment
krille-chan
left a comment
There was a problem hiding this comment.
@Tobias-Auer I've tried it out and made a first look at the code. This is awesome! I really want this! The first step to proceed would be to fix the linter rules:
'async' keyword is redundant, consider removing it.
Avoid returning widgets from a function.
And I will leave some comments on the code
| Future<void> _editImage(int index) async { | ||
| final bytes = await _readBytes(index); | ||
| if (!mounted) return; | ||
| final result = await Navigator.of(context).push<Uint8List?>( |
There was a problem hiding this comment.
Here I would prefer a app-specific method showScaffoldDialog() as this is intended to show a modal where a Scaffold is in it which looks good on mobile and on desktop mode:
final result = await showScaffoldDialog(
context: context,
builder: (_) => _ImageEditPage(bytes: bytes),
);
| ); | ||
| if (!mounted || result == null) return; | ||
| setState(() { | ||
| _editedBytes[index] = result; |
There was a problem hiding this comment.
Are we sure that this is sufficient? I guess the mimetype could also change?
Alternative idea: Instead of _editedBytes we store the whole XFile there as List<XFile> which is initiated from widget.files. When we edit a file we just create a new XFile.fromBytes(), make sure there that filename and mimetype are correct and this then switches out the previous XFile
| ); | ||
| } | ||
|
|
||
| Widget _buildBody() { |
There was a problem hiding this comment.
I think most of the Widget Functions can just directly moved into the build method widget tree. Abstracting a widget into something is usually done if:
a: It is used at different places (avoid code duplications)
b: The widget tree depth becomes too deep
But _buildBody() for example doesn't seem to be very deep in the widget tree but directly in the body of the scaffold so I'd just move it into it to get rid of the widget function
|
@Tobias-Auer you still wanna work on this PR? |
|
Yes, sorry didn't have much time last week due to driving school but I'll work on it this week |
4 participants
Adds image editor to the image send flow.
Resolves feature request #2218 and #3048
Pull Request has been tested on: