Skip to content
This repository was archived by the owner on May 23, 2025. It is now read-only.

Conversation

@nikclayton
Copy link
Contributor

@nikclayton nikclayton commented Jul 24, 2023

Previously the notification filter and clear actions were shown as buttons in the UI, with a preference that determined whether they were displayed.

Remove this preference, and display them as menu items.

  • "Filter notifications" is shown as an icon, if possible
  • "Clear notifications" is only ever shown as a menu item, to reduce the chance the user inadvertently selects it

To ensure that the options menu appears correctly, remove the code that creates a "fake" action bar, and adjust the layouts so that there are three toolbars;

  • mainToolbar -- displays the icons, and the current "location" (Home, Notifications, etc)
  • topNav -- displays the row of tabs at the top
  • bottomNav -- displays the row of tabs at the bottom

Only one of them is set as the support action bar (depending on the user's preferences). This provides the "show a logo" and "show the options menu" functionality as standard, without needing to re-implement as the previous code did.

Previously the notification filter and clear actions were shown as buttons in the UI, with a preference that determined whether they were displayed.

Remove this preference, and display them as menu items.

- "Filter" is shown as an icon, if possible
- "Clear" is only ever shown as a menu item

If the user has disabled the top navigation bar a new options menu item is shown to the right of the tabs, whether they are at the top or bottom of the screen.
@nikclayton
Copy link
Contributor Author

nikclayton commented Jul 24, 2023

Some screenshots of the new UI.

Here's with the topbar enabled, showing (a) the new filter shortcut icon, and (b) the options menu when it's open.

Here's what it looks like with the topbar disabled:

Same thing, but with the navigation at the bottom:

@charlag
Copy link
Collaborator

charlag commented Jul 30, 2023

I am not sure how I feel about options menu together with tabs but I support moving actions into toolbar & dropdown menu

@nikclayton
Copy link
Contributor Author

I am not sure how I feel about options menu together with tabs but I support moving actions into toolbar & dropdown menu

The menu is required (it should have been added when removing the top nav bar was added as an option) because you need a place to put functions that would otherwise be gesture-only.

Otherwise those functions are not accessible to eg screen reader users.

@Lakoja
Copy link
Collaborator

Lakoja commented Aug 14, 2023

Remove this preference, and display them as menu items.

I guess these changes need an issue?
There's a graphical / usage change that might need to be communicated.
And one might reference that issue in the settings migration comment (in TuskyApplication).

@Lakoja
Copy link
Collaborator

Lakoja commented Aug 14, 2023

On my emulator (Android 12) it looks like this at the moment:
grafik

So two problems:

  • It has no different background color and is thus melting with the timeline content.
  • The wording is a bit inconsistent.
    (Search is general, the other 4 are for the current list, consistently it should probably be called "Load newest" then

Both could be tackled in another issue/PR. However the first one might also be solved here.

Nik Clayton added 4 commits August 17, 2023 13:19
…ter-menu

# Conflicts:
#	app/src/main/res/values-cy/strings.xml
#	app/src/main/res/values-de/strings.xml
#	app/src/main/res/values-fa/strings.xml
#	app/src/main/res/values-gd/strings.xml
#	app/src/main/res/values-gl/strings.xml
#	app/src/main/res/values-is/strings.xml
#	app/src/main/res/values-ja/strings.xml
#	app/src/main/res/values-nl/strings.xml
#	app/src/main/res/values-oc/strings.xml
#	app/src/main/res/values-tr/strings.xml
#	app/src/main/res/values-vi/strings.xml
#	app/src/main/res/values-zh-rCN/strings.xml
Previous code either used setSupportActionBar (if the mainNav was enabled), or reproduced the logo/options menu functionality of an action bar if it was disabled.

There's no need to do that -- just have three toolbars (1 or 2 of which might be hidden), and use precisely one of them as the action bar.

Then you get logo support, navigation, and options menu handling for free.

This also simplifies the layouts and means you don't need to different bits of code for loading an image in to the action bar logo.
@nikclayton
Copy link
Contributor Author

@Lakoja @charlag

Turns out the code before this PR was overly complicated, and it's straightforward to have three toolbars (one for the mainnav, one for the top tabs, and one for the bottom tabs).

Whichever one of those is assigned to be the support action bar will display the logo and the options menu "for free", and does it with the correct theme/colours (which addresses one of your other comments).

I've adjusted the UI strings to make it clear these are operations on notifications as well.

I've updated the screenshots in the first comment on this PR to show the current behaviour.

PTAL.

@Lakoja
Copy link
Collaborator

Lakoja commented Aug 17, 2023

Whichever one of those is assigned to be the support action bar will display the logo and the options menu "for free", and does it with the correct theme/colours (which addresses one of your other comments).

Very nice!

PTAL.

PTAL? :-)

One of my comments is not yet resolved: This solves a rather noticeable bug (context menu not reachable) which should have an issue, I guess.

@nikclayton
Copy link
Contributor Author

PTAL.

PTAL? :-)

Please take a look.

One of my comments is not yet resolved: This solves a rather noticeable bug (context menu not reachable) which should have an issue, I guess.

I don't understand what you mean here.

@Lakoja
Copy link
Collaborator

Lakoja commented Aug 18, 2023

One of my comments is not yet resolved: This solves a rather noticeable bug (context menu not reachable) which should have an issue, I guess.

I don't understand what you mean here.

I gather then that we don't want to have issues for meaninful changes (for documentation / communication / release notes / ...)?

Here in this PR we have two quite distinct changes:

  • Notification filter button goes to context menu
  • Context menu is shown also when title is hidden

I have no reservations against doing both but I would document either change as an (one) issue.

@nikclayton
Copy link
Contributor Author

I gather then that we don't want to have issues for meaninful changes (for documentation / communication / release notes / ...)?

Not always, no. It can help, but it's not mandatory.

That's why it's important that the PR description stand alone in describing what has changed and why. If you look at recent PRs that I've merged from other people you'll see that I sometimes rewrite the PR description and/or title to try and make what's changed and why easier to understand.

Release notes are written by generating a list of the merged PRs since the previous tag (e.g., v23.0...develop) and reviewing each one to decide what to include in the release notes.

…ter-menu

# Conflicts:
#	app/src/main/java/com/keylesspalace/tusky/MainActivity.kt
@charlag charlag added this to the Tusky 24 milestone Nov 18, 2023
connyduck added a commit that referenced this pull request May 31, 2024
The option was removed in #3877,
then forgotten to be added again in
#4015.
#4026 added it again, but took to
long to merge, so we made #4225 as
well and that is how we ended up with the option twice.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants