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 Manga Notes #428

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Add Manga Notes #428

wants to merge 28 commits into from

Conversation

imkunet
Copy link

@imkunet imkunet commented Feb 19, 2024

Foreword

This is my first time ever working on anything Android related (the time of me downloading Android Studio is about two days ago), so any feedback on how to improve or pointing out any pitfalls is appreciated. This is also my first time contributing to a medium-sized open source project.

Manga Notes

What

This feature adds built-in per-manga markdown1 notes to help remember the details of manga inside and outside your library. It can be accessed through the top overflow menu or a button just below the tags through a button.

Why

Picture this: you have 50 or so forgettable isekai2 manga in your library. Because they are all in fact the same story with slight modifications, you lose track of the individual differences in each manga.

With the notes feature, you can add small (or large) reminders about the plot to each (easily) forgettable manga to make the reading experience of new chapters not completely horrible.

Intentions

I will commit to helping maintain this feature (when necessary) and improve this feature as to not place undue burden upon existing project maintainers.

Known issues

  • No collapsing/expanding notes on manga screen
    Addressed: Recent design revisions have the manga notes in the collapsible description section
  • Manga screen lazy list "jumps" when there is an extraordinarily long note Addressed: it's in the desc box now
  • No limit on note size (in theory could work correctly with notes 2^31-1 characters long, but not tested)
    Addressed: Imposed a 10k length limit with a visible warning when close to limit (tested) (it is now 250)
  • AOSP keyboard hates all Mihon TextFields and its auto-suggestion selects 1 space character extra behind the current word (not related to this feature itself)
  • Possibly conflicts with Edit basic Manga information #398

Demonstration

Old video
Revised.mp4

(latest video)

animations.mp4

Footnotes

  1. Markdown was chosen over something like rich text for being just enough for formatting if ever needed without requiring any additional bundle overhead without interfering with normal text.
    EDIT: The current revision uses rich text (HTML) for compatibility with the WYSIWYG editor EDIT 2: Back to markdown!

  2. Meaning: "Another world"

@imkunet imkunet marked this pull request as ready for review February 19, 2024 09:27
@imkunet
Copy link
Author

imkunet commented Mar 5, 2024

This comment is no longer relevant.

With the new rebase on main as well as the 0e0a9aa (Make spotless) commit (as well as the detekt gradle task) the information in this comment no longer makes any sense.

Its contents are hidden here for posterity.

Hidden

Commenting on the remaining failing ./gradlew detekt violations after ce9a3ac1:


This is a pre-existing issue with this function.

/mihon/data/src/main/java/tachiyomi/data/manga/MangaMapper.kt:8:17 [LongParameterList]
The function mapManga(...) has too many parameters. The current threshold is set to 6.

This is a pre-existing issue with this function.

/mihon/data/src/main/java/tachiyomi/data/manga/MangaMapper.kt:58:24 [LongParameterList]
The function mapLibraryManga(...) has too many parameters. The current threshold is set to 6.

This is a pre-existing issue with this data class.

/mihon/app/src/main/java/eu/kanade/tachiyomi/data/backup/models/BackupManga.kt:43:18 [MagicNumber]
This expression contains a magic number. Consider defining it to a well named constant.

Pretty much all the previews in the codebase have the same issue.

/mihon/app/src/main/java/eu/kanade/presentation/manga/components/MangaNotesSection.kt:96:13 [UnusedPrivateMember]
Private function MangaNotesSectionPreview is unused.


Let me know if there are any important issues that need to be fixed.

Footnotes

  1. detekt errors re-formatted for reading convenience

@CrepeTF
Copy link
Contributor

CrepeTF commented Mar 24, 2024

I've created a mock-up for this.

User interaction would be:
View -> (Sheet or fullscreen page presenting the full note)
↳ Edit notes -> Save

Mockup

@Turhvjbufv
Copy link

Turhvjbufv commented Mar 24, 2024

Why not view and edit notes buttons?(Instead of only the view button)
Seems like an extra step for nothing

@imkunet
Copy link
Author

imkunet commented Mar 24, 2024

Hi @CrepeTF .

I'm not sure if putting it in a outlined rounded rectangle is a good idea.

The padding on the sides removes vertical space that can be used for text and the button is quite distracting as I believe the text of your notes are should be first and foremost.

On the other hand, I like how attention grabbing it is and draws your attention to whatever you wrote about the manga. Perhaps there is a middle ground somewhere? It might be weird to have a big colored notes box when there are no notes added yet.

@imkunet
Copy link
Author

imkunet commented Sep 19, 2024

Rebased off of main, prevent silly merge commit from messing everything up. :)

@imkunet imkunet marked this pull request as draft September 19, 2024 07:23
@AntsyLich
Copy link
Member

Good to see you're still motivated despite me not reviewing. If you can hop on to Mihon discord for design discussion that'll be great.

@imkunet
Copy link
Author

imkunet commented Sep 20, 2024

Updated demonstration video with recent changes:

Preview.mp4

@KairuDeibisu
Copy link

Is this still being worked on?

@imkunet
Copy link
Author

imkunet commented Jan 19, 2025

There has been discussion in the Discord right around the time I stopped working on this about the exact character limit, particularly opposing thoughts on whether markdown or html or richtext should be used, or whether the characters are to be limited to 250. At the moment, I'm not quite sure what to do... though later today I might do a rebase and recheck the sentiment in the Discord chat with respect to the current feature set.

Thank you Syer10 for recommending this library
- Already built component for elegant rich text editing
- Adds a dependency, could possibly remove another (NewUpdateScreen.kt's
markdown viewer)
@imkunet
Copy link
Author

imkunet commented Jan 19, 2025

This is probably ready for review pending any last suggestions for changes left. The character has a soft limit of 250 (which is limited by the "characters" in the editor box), and is transparently stored as markdown (unbeknownst to the user) so if in the future there arises a need to migrate over to another system, there should be little pain in parsing and migrating.

@imkunet imkunet marked this pull request as ready for review January 19, 2025 23:32
@AntsyLich
Copy link
Member

Is this UI same as the last video you sent?

@nakedlittlezombie
Copy link

nakedlittlezombie commented Jan 26, 2025 via email

@imkunet
Copy link
Author

imkunet commented Jan 27, 2025

It's mostly the same aside from a string change and removing the heading button. Current UI:

HEAD.mp4

@imkunet imkunet requested a review from AntsyLich January 27, 2025 06:04
@imkunet
Copy link
Author

imkunet commented Jan 27, 2025

animations.mp4

Copy link
Member

Choose a reason for hiding this comment

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

Just put all the content in the Screen file as a subclass

class MangaNotesScreen(
    private val manga: Manga,
) : Screen() {
    ...
    private class Model(
         ...
    ) {
        ...    
    }

    data class State(
        ...
    )
}

Comment on lines +32 to +36
if (!isFirstUpdate) {
alpha.snapTo(targetValue = 0f)
alpha.animateTo(targetValue = 1f, animationSpec = tween(500))
return@LaunchedEffect
}
Copy link
Member

Choose a reason for hiding this comment

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

Just put the whole thing inside LaunchedEffect(Unit)

richTextState.config.linkColor = primaryColor
}

// to prevent the content size from animating from first render
Copy link
Member

Choose a reason for hiding this comment

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

What's the issue with that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants