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

[GTK] Improve "play episode" context menu #616

Merged
merged 3 commits into from
Jun 2, 2022

Conversation

imsamuka
Copy link
Contributor

Fixed media with 0 or unknown total episodes/chapters showing an empty menu

Before

Anime with unknown total episodes before

After

Anime with 0 episodes after
Anime with unknown total episodes after

Changed menu style, and added a "present in library" indicator

Before

style before

After

style after

@imsamuka imsamuka marked this pull request as ready for review May 22, 2022 23:26
@z411 z411 merged commit 934c567 into z411:master Jun 2, 2022
@z411
Copy link
Owner

z411 commented Jun 2, 2022

Looks nice. Thank you.

mb_playep.set_label(str(i) + " - Next")
mb_playep.set_margin_left(10)
menu_eps.set_focus_child(mb_playep)
mb_playep.set_inconsistent(i < next_ep)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd probably switch that around and have the "inconsistent" state for not yet watched episodes while the watched ones are checked normally.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd probably switch that around and have the "inconsistent" state for not yet watched episodes while the watched ones are checked normally.

In isolation, i would agree, but i don't. Seeing 7/12 anywhere already states pretty obviously that you watched 7 episodes. It's not very useful to have the check boxes saying the same thing.
I did highlight the "next" episode, so it's very easy to find the boundary on shows with low episode count.

The inconsistent state is somewhat useful when you need to scroll down on shows with more than 100 episodes, because it's harder to find the next episode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In isolation, i would agree, but i don't.

Please elaborate, I'm not following?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Outside" the context menu, you can already see the current show progress in multiple places. If we ignore those (take the context menu in isolation), i would agree with your suggestion too.

Copy link
Contributor

@ahmubashshir ahmubashshir Jun 10, 2022

Choose a reason for hiding this comment

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

@imsamuka this is the correct way(active: done, inactive: not done, inconsistent: in between)
Peek 2022-06-10 22-07

diff --git a/trackma/ui/gtk/mainview.py b/trackma/ui/gtk/mainview.py
index 484ac98..83eee1b 100644
--- a/trackma/ui/gtk/mainview.py
+++ b/trackma/ui/gtk/mainview.py
@@ -757,13 +757,15 @@ class NotebookPage(Gtk.ScrolledWindow):
         menu_eps = Gtk.Menu()
         for i in range(1, total + 1):
             mb_playep = Gtk.CheckMenuItem(str(i))
-            if i == next_ep:
+            if i < next_ep:
+                mb_playep.props.active = i in library_episodes
+            elif i == next_ep:
                 mb_playep.set_label(str(i) + " - Next")
                 mb_playep.set_margin_left(10)
                 menu_eps.set_focus_child(mb_playep)
-            mb_playep.set_inconsistent(i < next_ep)
-            mb_playep.set_active(i in library_episodes)
-            mb_playep.set_draw_as_radio(True)
+            else:
+                mb_playep.props.inconsistent = i in library_episodes
             mb_playep.connect("activate",
                               self._on_mb_activate,
                               ShowEventType.PLAY_EPISODE, i)

Copy link
Contributor

Choose a reason for hiding this comment

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

And sorry for the late reply, I was busy for uni admission prep. today was the first one, a few more left(the next one is probably at the end of this month, I'll be down in a few days again...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FichteFoll @ahmubashshir I get what you mean.

My biggest point here, is that we don't need mb_playep.props.active to indicate "watched" episodes. When you see the "Next" episode, you can already tell that every episode before, was already watched. I used mb_playep.props.inconsistent to show it as well, but seeing how counterintuitive this design choice was, i propose to remove the inconsistent state completely and do something else:

Suggested change
mb_playep.set_inconsistent(i < next_ep)
elif i > next_ep:
mb_playep.set_margin_left(10)

Peek 2022-06-11 14-29

Now mb_playep.props.active only means "episode downloaded" and it's still pretty easy to find the "Next" episode (on shows with too many episodes) because of the indentation/left margin. No more "inconsistent" state messing everyone up.

I really wanted to make the "Next" episode already focused when you open the menu, so that you wouldn't need to scroll down, but i couldn't find how.

Copy link
Collaborator

@FichteFoll FichteFoll Jun 12, 2022

Choose a reason for hiding this comment

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

When you see the "Next" episode, you can already tell that every episode before, was already watched.

That is true, but if that bothers you or someone we need to talk about usage patterns of this menu in the first place to understand what it can be used for and how to best achieve that.

  • When you want to play the "next" episode only, then you should be using the separate menu entry for that.
  • When you want to play an arbitrary episode of a show with <24 episodes, the menu does its job and displaying which episodes you have downloaded exactly is useful as the functionality wouldn't be available for the others. As such, it would be fine to remove the intermediary state again and instead just mark the downloaded episodes.
  • When you want to play an arbitrary episode of a show with >24 episodes is bothersome and an input prompt would be much easier. This is also how it works on the CLI, which I use primarily.
  • When you want to play an episode in close proximity to the last one you watched/the next episode, then the changes you made to the menu text are sufficient to make it stand out, alas only for shows with <24 episodes as anything else requires scrolling to find that one episode. In that case, a dialog as proposed previously would also work.

The actionale steps I would extract from this are:

  1. Remove the "tri-state" styling since the menu is primarily concerned about showing which episodes can be played and have been downloaded and we have an indicator already indicating the next episode. We can still debate whether we want the styles to be "unchecked and checked" in a checkbox or "unchecked and intermediary" in a radio.
  2. Add a dialog prompt for shows with more than X episodes because the menu effectively becomes unusable. Take One Piece with over 1000 episodes, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfectly said. Regarding the 4 usage patterns, the fourth one is primarily my use case. More often than anything, I see myself playing the previous, or ~3 episodes before the next one. Actually, I don't now if there's anyone hoping to watch, like, 2 episodes after the next one...

...alas only for shows with <24 episodes as anything else requires scrolling to find that one episode.

As you can see in the GIF i sent, scrolling down 50, is still quite fast. Over 100 episodes, it becomes pretty annoying. If the menu was already scrolled down to the Next episode whenever opened, even in long lists, it would also be quite fine.

Add a dialog prompt for shows with more than X episodes because the menu effectively becomes unusable. Take One Piece with over 1000 episodes, for example.

Yes, and since you can't see which episodes you have downloaded, i propose something like this multi-column combo box instead of a simple input prompt. I will experiment with this right away.

There's also this idea:

Multi-Column Submenu

If even possible in GTK, using my GIF as a reference, it probably can accommodate up to ~130 episodes (26 * 5 columns) on screen at the same time, at that point, even One Piece would only take ~8 screens (1000 / 130). Still quite annoying to scroll down, but way more manageable. With more columns, it can be even better.

I would still use a single list in animes with <27 episodes, though.

@imsamuka imsamuka deleted the improve-play-menu branch August 7, 2022 03:34
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.

4 participants