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

added article summary with OpenAI integration #399

Merged
merged 7 commits into from
Nov 16, 2024

Conversation

anod
Copy link
Contributor

@anod anod commented Oct 24, 2024

Add summarize feature which fetching an article content and summarizes it using OpenAI API.
Api key needs to be provided in the settings

image
image
image

@anod
Copy link
Contributor Author

anod commented Oct 24, 2024

@spacecowboy please take a look

@spacecowboy spacecowboy self-requested a review October 24, 2024 21:43
@spacecowboy
Copy link
Owner

Sure. Let me know when you think the functionality is done.

The structure seems reasonable. I noted some hard-coded english strings and some lack of padding in layouts, but I'll hold off on reviewing in detail until later.

What issues are you having with "theming of buttons and progress bars"?

@anod anod force-pushed the dev/anod/summarize branch from 751bef2 to 163fb61 Compare October 26, 2024 10:12
@anod anod marked this pull request as ready for review October 26, 2024 10:13
@anod
Copy link
Contributor Author

anod commented Oct 26, 2024

Sure. Let me know when you think the functionality is done.

The structure seems reasonable. I noted some hard-coded english strings and some lack of padding in layouts, but I'll hold off on reviewing in detail until later.

What issues are you having with "theming of buttons and progress bars"?

theme is not being applied
image
image

@anod anod changed the title [Draft] Article summary -OpenAI Integration Article summary - OpenAI Integration Oct 26, 2024
@spacecowboy
Copy link
Owner

Theme isn't applying because you are importing the wrong assets.

import androidx.compose.material.LinearProgressIndicator

Only import material3 assets.

Exception to that is things from the icons library. But everything else should only be material3

@anod anod force-pushed the dev/anod/summarize branch from 163fb61 to 9f26a30 Compare November 2, 2024 12:36
@anod
Copy link
Contributor Author

anod commented Nov 2, 2024

Thanks forgot about material 1

@spacecowboy spacecowboy marked this pull request as draft November 3, 2024 02:39
@spacecowboy
Copy link
Owner

Converted the PR to a draft. Please let me know when it's ready for review

@anod
Copy link
Contributor Author

anod commented Nov 3, 2024

Converted the PR to a draft. Please let me know when it's ready for review

What's missing? I don't plan to add anything

@spacecowboy
Copy link
Owner

spacecowboy commented Nov 5, 2024

Converted the PR to a draft. Please let me know when it's ready for review

What's missing? I don't plan to add anything

Well CI is failing for one.

  • Please run ./gradlew ktlintformat
  • Please remove all hard coded english strings and make proper string resources
  • Fix material1 imports to be material3

@anod anod force-pushed the dev/anod/summarize branch from 9f26a30 to b1a7410 Compare November 7, 2024 07:32
@anod
Copy link
Contributor Author

anod commented Nov 7, 2024

@spacecowboy Since I can't run CI, please run and let me know if it passes

@spacecowboy
Copy link
Owner

@spacecowboy Since I can't run CI, please run and let me know if it passes

Flipped a switch so it should run automatically from now on

@spacecowboy
Copy link
Owner

Also, please stop force-pushing. It makes it hard to understand what changes.

I will squash merge your PR when/if it's merged anyway

@anod
Copy link
Contributor Author

anod commented Nov 8, 2024

Also, please stop force-pushing. It makes it hard to understand what changes.

I will squash merge your PR when/if it's merged anyway

I use rebase, I prefer my changes to be a single commit on top any other change and not spread across timeline, you can see and review all the changes in "Files changes" tab

@anod anod marked this pull request as ready for review November 8, 2024 06:26
Copy link
Owner

@spacecowboy spacecowboy left a comment

Choose a reason for hiding this comment

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

I think it can be a cool feature to the app. Thanks for working on it!

I have noted some issues inline, and there is a problem with the settings layout on tablets:

tablet_margins

But the biggest thing is the settings UX. It has a save/cancel button in a UI where everything else saves directly on edits.

As I see it, it should either:

  • be changed so it's only the Editable section that's used. And save the fields directly as they are edited. But that may be non-ideal.
  • or change so that the OpenAI Integration section only has one settings, a text button which opens a new screen/dialog, in which a save and cancel buttons can be present. Kind of how the Edit feed screen works. The Block list setting works like this and opens a popup. While Device sync opens an entire screen.

What to you think?

@anod
Copy link
Contributor Author

anod commented Nov 14, 2024

I think it can be a cool feature to the app. Thanks for working on it!

I have noted some issues inline, and there is a problem with the settings layout on tablets:

tablet_margins

But the biggest thing is the settings UX. It has a save/cancel button in a UI where everything else saves directly on edits.

As I see it, it should either:

* be changed so it's only the Editable section that's used. And save the fields directly as they are edited. But that may be non-ideal.

* or change so that the `OpenAI Integration` section only has one settings, a text button which opens a new screen/dialog, in which a `save` and `cancel` buttons can be present. Kind of how the `Edit feed` screen works. The `Block list` setting works like this and opens a popup. While `Device sync` opens an entire screen.

What to you think?

Will change to dialog, I just don't like dialogs as a UX pattern and with Compose it much easier to have inline experience, but I agree that it might be better for familiarity

Also I have more ideas for integration:

  1. Summarize in specific language
  2. Add follow-up options: give more details, or shorten
  3. Integrate with catalog to summarize titles

@spacecowboy
Copy link
Owner

Sorry for the delay in review. I think this looks good now. Thanks for your hard work 👍

@spacecowboy spacecowboy changed the title Article summary - OpenAI Integration added article summary with OpenAI integration Nov 16, 2024
@spacecowboy spacecowboy enabled auto-merge (squash) November 16, 2024 18:43
@spacecowboy spacecowboy merged commit ca13fd4 into spacecowboy:master Nov 16, 2024
2 checks passed
@anod anod deleted the dev/anod/summarize branch November 26, 2024 06:57
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.

2 participants