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

Dedicated title/description input fields #46

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

tuancoltech
Copy link
Collaborator

@tuancoltech tuancoltech commented Jan 7, 2024

@tuancoltech tuancoltech self-assigned this Jan 7, 2024
@tuancoltech tuancoltech added the enhancement New feature or request label Jan 7, 2024
Copy link
Member

@shubertm shubertm left a comment

Choose a reason for hiding this comment

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

It would also be better to allow user save title and description separately, right now title and description are tightly coupled to content despite having separate storage #39

@kirillt
Copy link
Member

kirillt commented Jan 10, 2024

@ShubertMunthali not sure what do you mean, I see that the title is stored into new .ark/user/properties storage as JSON. It seems to allow empty description and/or empty title.

@tuancoltech the feature works good, the only issue is that it's not possible to modify title after initial creation if the content isn't changed. Because [save] button doesn't allow saving same note twice. It shouldn't update the content, but any properties updates should be saved.

@kirillt kirillt changed the title [enhancement] Dedicated title/description input fields Dedicated title/description input fields Jan 11, 2024
@tuancoltech
Copy link
Collaborator Author

@ShubertMunthali not sure what do you mean, I see that the title is stored into new .ark/user/properties storage as JSON. It seems to allow empty description and/or empty title.

@tuancoltech the feature works good, the only issue is that it's not possible to modify title after initial creation if the content isn't changed. Because [save] button doesn't allow saving same note twice. It shouldn't update the content, but any properties updates should be saved.

@kirillt At the moment, if we modify a note's content, a new note will be created instead of overwriting the existing note file.
Do we want to duplicate the note files, or overwriting in the same file in case user modify the note's description and/or title?

@shubertm
Copy link
Member

@ShubertMunthali not sure what do you mean, I see that the title is stored into new .ark/user/properties storage as JSON. It seems to allow empty description and/or empty title.

@tuancoltech the feature works good, the only issue is that it's not possible to modify title after initial creation if the content isn't changed. Because [save] button doesn't allow saving same note twice. It shouldn't update the content, but any properties updates should be saved.

@kirillt I mean what you just explained, that save button does not work if only properties are edited, it works when content is edited. we need it to work with properties edited alone, or with properties edited together with content. That's what is required in #39

@shubertm
Copy link
Member

@ShubertMunthali not sure what do you mean, I see that the title is stored into new .ark/user/properties storage as JSON. It seems to allow empty description and/or empty title.
@tuancoltech the feature works good, the only issue is that it's not possible to modify title after initial creation if the content isn't changed. Because [save] button doesn't allow saving same note twice. It shouldn't update the content, but any properties updates should be saved.

@kirillt At the moment, if we modify a note's content, a new note will be created instead of overwriting the existing note file. Do we want to duplicate the note files, or overwriting in the same file in case user modify the note's description and/or title?

If only properties change we should overwrite, but whenever content changes we need a new file. Version tracking in #13 will handle the duplicates properly.

@kirillt
Copy link
Member

kirillt commented Jan 11, 2024

Yep, @ShubertMunthali is right here. We're missing ability to edit title/description when the content is untouched. We don't need to save the content twice in this case, only update user/properties storage for the current resource id.

@tuancoltech
Copy link
Collaborator Author

Yep, @ShubertMunthali is right here. We're missing ability to edit title/description when the content is untouched. We don't need to save the content twice in this case, only update user/properties storage for the current resource id.

@ShubertMunthali @kirillt Thanks for the idea, guys. I'm making changes to align with this.

@tuancoltech
Copy link
Collaborator Author

tuancoltech commented Jan 11, 2024

@ShubertMunthali @kirillt Please help reviewing again.
Title/description are now validated and stored independently from content.

@kirillt
Copy link
Member

kirillt commented Jan 14, 2024

@tuancoltech Good, but couple of nitpicks:

  • If we create a note without providing title or description, the empty JSON is written to the properties storage: {"titles":[""],"descriptions":[""]}. Can we avoid it in the app by simply treating absent properties file as empty JSON?

    • If we need to fix arklib-android for this, then let's create the issue there.
    • If we can fix it only in the app code, let's do it.
  • Graphical notes are modified in weird way when their title or description is modified.

Steps:

  1. Create new graphical note and save.
  2. Open it and change the title, save.
  3. Open it again and change the description, save.

Result: 3 different SVG files.

$ xmllint --format 2035-514095390.svg > /tmp/a
$ xmllint --format 2035-1319256942.svg > /tmp/b
$ xmllint --format 2035-2093532239.svg > /tmp/c
$ diff /tmp/{a,b}
2c2
< <svg xmlns="http://www.w3.org/2000/svg" viewBox="0.0 0.0 1440.0 2550.0">
---
> <svg xmlns="http://www.w3.org/2000/svg" viewBox="0.0 0.0 1440.0 1535.0">
[kirill@lenovo Notes]$ diff /tmp/{b,c}
2c2
< <svg xmlns="http://www.w3.org/2000/svg" viewBox="0.0 0.0 1440.0 1535.0">
---
> <svg xmlns="http://www.w3.org/2000/svg" viewBox="0.0 0.0 1440.0 1398.0">

For some reason, viewBox property is updated.

@tuancoltech
Copy link
Collaborator Author

  • If we create a note without providing title or description, the empty JSON is written to the properties storage: {"titles":[""],"descriptions":[""]}

@kirillt For the empty JSON issue, we could only handle that from arklib-android.
I created an issue for it here: ARK-Builders/arklib-android#137

For the second issue, it's because we are updating the viewBox of current SVG whenever there's size change of the drawing view as per in below screenshot.
I'm clarifying this with @ShubertMunthali .

Screenshot 2024-01-16 at 12 40 04

@kirillt
Copy link
Member

kirillt commented Jan 16, 2024

Thank you @tuancoltech, that's perfect.

@kirillt
Copy link
Member

kirillt commented Jan 16, 2024

So, I guess it's easy to fix as soon as @ShubertMunthali share some info and we'll merge then.

@shubertm
Copy link
Member

We need to set dimensions for newly created note only here. @tuancoltech that's a bug indeed, we need a fix. You can provide it, if yo don't mind

@tuancoltech
Copy link
Collaborator Author

We need to set dimensions for newly created note only here. @tuancoltech that's a bug indeed, we need a fix. You can provide it, if yo don't mind

@ShubertMunthali Definitely, I have fixed it by removing the unnecessary onSizeChanged method.
Please re-review @kirillt !

Copy link
Member

@kirillt kirillt left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@kirillt kirillt merged commit c8a5468 into main Jan 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants