Skip to content

fix: hide save button in fragmented activity to prevent duplication#18951

Merged
lukstbit merged 1 commit intoankidroid:mainfrom
Haz3-jolt:Duplicate
Aug 4, 2025
Merged

fix: hide save button in fragmented activity to prevent duplication#18951
lukstbit merged 1 commit intoankidroid:mainfrom
Haz3-jolt:Duplicate

Conversation

@Haz3-jolt
Copy link
Member

@Haz3-jolt Haz3-jolt commented Jul 23, 2025

Purpose / Description

  • Hides the save button from NoteEditorFragment in cardBrowser to prevent duplication

Fixes

Approach

  • The cardBrowser already has a 'save_note' being added, and noteEditorFragment adds another one.
  • Here we introduce a check to prevent additional 'save_note' buttons from being added.
  • This occurred after the NoteEditor was separated into the NoteEditorActivity.
  • Since NoteEditorFragment is a MenuProvider we do not need onCreateMenu anymore.

How Has This Been Tested?

image

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@Haz3-jolt Haz3-jolt added Needs Review GSoC Pull requests authored by a Google Summer of Code participant [Candidate/Selected], for GSoC mentors labels Jul 23, 2025
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

I don't feel the issue, PR or commit message explain why this is occurring.

What's producing the other save button? Why are we removing it from the Note Editor?

@Haz3-jolt
Copy link
Member Author

I don't feel the issue, PR or commit message explain why this is occurring.

What's producing the other save button? Why are we removing it from the Note Editor?

I've added some context in the PR, I'll also do so in the commit message

@david-allison
Copy link
Member

david-allison commented Jul 23, 2025

I'd remove adding the menu item 'action_save' from the Browser. It should be a responsibility of the note editor.

The Browser should instead override the action_save, instead of passing it down to the fragment

Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

In the commit message "save note" and "preview" should be quoted. Or even you should state that it remove the menu entry named "save note" and "preview". As those are verbs that are used as noun, it makes the sentence very confusing otherwise.

I really don't understand what the last line mean. I have now idea which check you're referring to. How could a check disappear.

I appreciate that your commit message are longer than a single line, but it's still hard to follow.

@Haz3-jolt
Copy link
Member Author

Should have a more clear message rn! Thanks for the heads up, it was a lot more obvious to me because I worked on it!
Hindsight blinded me ig 😆

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Ok, consider making the title ~50 chars:

fix(card-browser): remove duplicate menu items

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jul 29, 2025
- Since NoteEditorFragment is now a MenuProvider, it automatically contributes its menu items when attached. The manual call to fragment?.onCreateMenu(...) is now redundant and has been removed to prevent duplicate “Save Note” and “Preview” entries in X-Large layout

- Added a check to remove the “Save Note” and “Preview” actions when there are no notes in the CardBrowser
Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

Let's get this in...

@lukstbit lukstbit added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Aug 4, 2025
@lukstbit lukstbit added this pull request to the merge queue Aug 4, 2025
Merged via the queue into ankidroid:main with commit e436b29 Aug 4, 2025
10 checks passed
@github-actions github-actions bot added this to the 2.23 release milestone Aug 4, 2025
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

GSoC Pull requests authored by a Google Summer of Code participant [Candidate/Selected], for GSoC mentors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate Save Note buttons when NoteEditor is in X-Large CardBrowser and preview icon missing

5 participants