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

UpdateChangelogTask broken #5606

Closed
westnordost opened this issue Apr 29, 2024 · 3 comments
Closed

UpdateChangelogTask broken #5606

westnordost opened this issue Apr 29, 2024 · 3 comments
Labels

Comments

@westnordost
Copy link
Member

westnordost commented Apr 29, 2024

How to Reproduce
Run the UpdateChangelogTask. Notice that the html looks weird, i.e. starts like this:

<h1>Changelog
</h1><p>
</p><h2>v57.4
</h2>

Subsequently the subsequent transformation doesn't work as expected: "Changelog" is not stripped and the last version for the changelog is not found.

Expected Behavior
Either make it work as before (no idea what changed that it suddenly does not work correctly anymore. Does the markdown parser not handle \r\n properly?) ...

or

ditch this HTML stuff, because:

  • bending the webview display to look natively like the app is somewhat awkward and more code, more difficult to migrate to Compose (and this was the goal of this whole excercise)
  • for me at least, the webview solution doesn't work very well - I can always scroll horizontally
  • also, an official webview for compose does not exist and is not planned by Jetbrains or Google
  • the "changelog is too large to display in a normal textview"-issue can be solved by pagination. E.g. have buttons like
    [<< v53.1] [v54.0] [v54.1 >>] or similar (a dropdown?) at the bottom to select for which version one wants to view the changelog. Would be more convenient too, I'd say
  • it is quite trivial to programmatically build a UI (header, text, list items, ...) via Compose from structured data. E.g. let's say you have a list of list items, then you can just write
for (string in listItems) {
  ListItem(string)
)

(ListItem being a Composable that would be defined in a few lines of code, basically a text to the end of a •)

@FloEdelmann

@FloEdelmann
Copy link
Member

Indeed, \r\n was the culprit. Since files only have \n by default on Linux, I didn't think it could be an issue.

bending the webview display to look natively like the app is somewhat awkward and more code, more difficult to migrate to Compose (and this was the goal of this whole excercise)

Agreed, it's a hacky workaround.

for me at least, the webview solution doesn't work very well - I can always scroll horizontally

Could you please record a quick screencast? I have a CSS quickfix in mind, but I want to reproduce it locally first.

also, an official webview for compose does not exist and is not planned by Jetbrains or Google

That's bad, but I think we need a webview for the login flow anyway, right?

@westnordost
Copy link
Member Author

westnordost commented Apr 30, 2024

Thanks for the fix!

Could you please record a quick screencast? I have a CSS quickfix in mind, but I want to reproduce it locally first.

I can only scroll vertically a bit, a few pixels, maybe 30dp.

also, an official webview for compose does not exist and is not planned by Jetbrains or Google

That's bad, but I think we need a webview for the login flow anyway, right?

Currently, yes.
As noted here #5607 (comment) -

Login currently uses the WebView, for which there is no official Compose replacement. There is a library by a third party, but it misses at least one crucial part of the API to be used for OAuth authorization - shouldOverrideUrlLoading - used for retrieving the authorization code from the callback URL. So, maybe it is better to use the browser for OAuth authorization instead of a webview (which is also recommended in OAuth 2 anyway) but in any case I will postpone this because of that.

it is an option to change the login flow to using the external browser. The reason I changed it from initially being in the browser to later being in a WebView was primarily that it did not work with the LineageOS stock browser, IIRC. At least with LineageOS 18 (current is 24) it was still a problem: https://gitlab.com/search?search=ERR_UNKNOWN_URL_SCHEME+&nav_source=navbar&project_id=9202919&group_id=3936439&scope=issues

@westnordost
Copy link
Member Author

Anyway, parsing the markdown in-app and then displaying this in a nice composable sounds like a nice little project we could do together for me. You learn a bit how to work with Compose and we eliminate the need for a WebView at that point in the app. Currently, the transformation into an HTML and displaying that HTML in a WebView is about 100 lines of code. An implementation in compose will probably not be much longer than that. My estimation would be around 200 lines for everything (i.e. including layout that would be in a layout XML in the classic Android View system approach).

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

No branches or pull requests

2 participants