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

Reworked attachments #2106

Merged
merged 19 commits into from
Oct 10, 2023
Merged

Reworked attachments #2106

merged 19 commits into from
Oct 10, 2023

Conversation

harshad1
Copy link
Collaborator

In this PR I have reworked the attachments system to fix bugs and reduce clicks.

  • The attachment actions have been broken out of a nested menu into the main actions bar
  • Camera, gallery and recording attachments add immediately when clicking ok. They can be edited later if needed.
  • Camera permissions fixed
  • User no longer given choice for where to save attachment
    • Setting include option for default attachment folder name
    • Need to consider making this per-file

// Create an image file name
if (target != null && !target.isDirectory()) {
photoFile = target;
public void requestCameraPicture(final Activity activity, final File targetDir) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simplified code

@harshad1
Copy link
Collaborator Author

@justpeanuts

Hi, I tested it, it works well! My only comments on it are:
i) the 3 clicks that are required now could be minimised to just two clicks if the "insert image from camera" and "insert image from filesystem" were made into two separate toolbar options.
ii) actually, in effect, it requires a fourth click, because keyboard does not open by default after inserting the image link. It would be great if keyboard popped up with the cursor being at new line by having a line-break after the image-link.
iii) the filename of the image taken from the camera is like this : IMG_20230912-152630.jpg5470420763913322814.jpg - nothing wrong, just seemed a little weird.
iv) it would be superb if there was another "batch-image-add" option to take multiple images form the camera and markor automatically makes a pdf out of it and attaches the link to the pdf. (But i understand that that would probably be way too much work)
v) "Marder" really..? you gave me a scare there.. lol.

  1. It's a balance between being too nested and having too many options at the top level. I'll continue to iterate on this in the future based on tickets and usage patterns.
  2. That's a bug :). I'll look in to fixing it. Keyboard should maintain its state.
  3. That's another bug :). Thanks for noticing.
  4. I will be looking into batch adding images from the gallery in a follow up PR at some stage.
  5. It used to be called 'Mordor', which I kind of liked. Not sure why it was changed....

@harshad1
Copy link
Collaborator Author

harshad1 commented Oct 2, 2023

@gsantner Images no longer render in view mode with the latest master. Do we know what might have broken?

@harshad1
Copy link
Collaborator Author

harshad1 commented Oct 3, 2023

@gsantner @guanglinn I have addressed the image issue in the latest commit

@gsantner
Copy link
Owner

gsantner commented Oct 7, 2023

Great, didn't even notice regarding images ... as I use the release version mainly 😄

@harshad1
Copy link
Collaborator Author

harshad1 commented Oct 8, 2023

Yeah, I like using a branch with all my current PRs. It's the best way to find bugs.

This branch is ready to review + merge imo.

@gsantner
Copy link
Owner

Thank you for all the changes & improvements, @harshad1 .

I installed it on device and at didn't notice something off.

I merge it now. Improvements are still welcome any time, and if it breaks something lets fix it.

@gsantner gsantner merged commit a20ea28 into gsantner:master Oct 10, 2023
1 check passed
gsantner pushed a commit that referenced this pull request Oct 31, 2023
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

2 participants