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

Idea: Optionally display tracks in an entry view #35

Closed
fossfreedom opened this issue Sep 24, 2012 · 22 comments
Closed

Idea: Optionally display tracks in an entry view #35

fossfreedom opened this issue Sep 24, 2012 · 22 comments
Assignees
Milestone

Comments

@fossfreedom
Copy link
Owner

This idea is useful (I think) - and should be easy to implement.

On right-click on an album icon/or multiselect albums have an GtkCheckMenuItem to display tracks.

Using the RBEntryView widget, display the widget between the icon-view and the status line.

Connect the widget to a RhythmDBQueryModel - but manually populate the model with the entries stored in the selected Album/Albums.

Thoughts on this?

@asermax
Copy link
Contributor

asermax commented Sep 24, 2012

Seems interesting :3
Just one though: the user should be able to close or hide the entry view (maybe using an expander). Otherwise, I think the ui would get way to cluttered.

EDIT: Oh btw, I'll be cleaning some code, adding some comments and stuff, before starting with a new issue.
Oh, and another thing, why are you creating new branches for each release? You don't like tags?

@ghost ghost assigned fossfreedom Sep 24, 2012
@fossfreedom
Copy link
Owner Author

@asermax - not sure about the expander thing - I've quickly knocked up the stuff in "entryview" - is this what you were referring to.

As to branches - sorry - its a habit I've got into in my day job - keep a couple of release branches as a project moves forwards, deleting older ones when not required - makes viewing code between releases a little easier.

@asermax
Copy link
Contributor

asermax commented Sep 24, 2012

Heh, no problem, I was just wondering :3 it's fine if it helps you keep things cleaner!

And yup, that's pretty much what i was talking about, sweet. Is there a way to show some text next to the expander to explain what is it for?

EDIT: something else that would be interesting is to use a GtkPaned to be able to distribute the space between the covers_view and the entry_view

@asermax
Copy link
Contributor

asermax commented Sep 25, 2012

I'm getting a lot of warnings on the console when running rhythmbox over your branch. I think there's some loose signal somewhere xD

@fossfreedom
Copy link
Owner Author

yep - I noticed that as well. I thought it would be as simple as adding "return True" at the end of the callbacks to indicate the signal had been handled. Still getting them. I'll continue investigating.

Added the gtkpane. Need to work-out though how to set the position of the expander to the bottom of the screen. gtk.set_position calculates in pixels from the top of the screen.

@asermax
Copy link
Contributor

asermax commented Sep 25, 2012

I like where this is going, keep working on it, and let me know if you need help with anything!

Btw, I just finished refactoring master, so I merged it into entryview to save you the problem of doing it yourself. Let me know if it causes any issue :T

@fossfreedom
Copy link
Owner Author

@asermax - will I'm baffled.

It looks like my changes to the ui file - by adding a GtkFrame and/or GtkPaned as parents to the GtkScrolledWindow causes this.

Possibly because the top widget is a GtkHBox ?

Dunno really. Any ideas?

@asermax
Copy link
Contributor

asermax commented Sep 25, 2012

I don't get it, what's the problem? The thing about the warnings?

@fossfreedom
Copy link
Owner Author

yes these errors:

(rhythmbox:21520): GLib-GObject-WARNING **: invalid (NULL) pointer instance
(rhythmbox:21520): GLib-GObject-CRITICAL **: g_signal_connect_object: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed
(rhythmbox:21520): GLib-GObject-WARNING **: invalid (NULL) pointer instance
(rhythmbox:21520): GLib-GObject-CRITICAL **: g_signal_connect_object: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed
(rhythmbox:21520): GLib-GObject-WARNING **: invalid (NULL) pointer instance
(rhythmbox:21520): GLib-GObject-CRITICAL **: g_signal_connect_object: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed
(rhythmbox:21520): GLib-GObject-WARNING **: invalid (NULL) pointer instance
(rhythmbox:21520): GLib-GObject-CRITICAL **: g_signal_connect_object: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

asermax added a commit that referenced this issue Sep 25, 2012
…the frame arround the entryview for a GtkBox, this makes lower pane to shrink to it's minimum size when the expander and entryview are hidden (issue #35)
@asermax
Copy link
Contributor

asermax commented Sep 25, 2012

Well, after some inspection, I pin-pointed the problem to the line where the GtkBuilder gets feed with the ui file:

            'coverart_browser.ui'))```
What surprised me is that it is that line, and not the next one that connects the signals. 
The only idea I've is that it may be something related with an incompatibility between widgets (like you said, maybe the paned doesn't something) OR those errors are related to the EntryView widget.

I would like to test further but I've to leave right now, if you are still trying to figure out this enigma by when I came back, I'll be glad to help d:

@fossfreedom
Copy link
Owner Author

Hmmm ... as soon as I removed

< child>
<object class="RBEntryView" id="entryview">
<property name="visible">True</property>
<property name="can_focus">True</property>
<property name="hexpand">True</property>
<property name="vexpand">False</property>
<property name="shadow_type">in</property>
</object>
</child>

The errors disappeared.

I'm wondering if this is an upstream issue.

Do you know if there is a way to add a RB.EntryView widget manually via code to the same position (? placeholder?) where RBEntryView is/was in the UI file? Maybe coding this will remove the errors.

@asermax
Copy link
Contributor

asermax commented Sep 25, 2012

I think it's not actually a bug; the thing is, if you check up the RBEntryView constructor's signature, it needs an instance of RhythmDB and ShellPlayer. I dont't think there is a way to pass this parameters through GtkBuilder (for which I wonder why they marked this widget as GtkBuildable...)
Anyway, yes, there's a way. Just leave the box empty, and in the activation method retrieve this box (entryview_box iirc) and add it as a child (I think the add method should work fine, but if not, try with one of pack's variants). Let me know how it goes!
PS: sorry if there are errors on this post, I'm writing from my phone xD

@fossfreedom
Copy link
Owner Author

I've sent this to the rhythmbox dev mailing list - looks to me like an upstream bug

from gi.repository import RB
db = shell.props.db
shell_player = shell.props.shell_player
entry_view = RB.EntryView().new(shell.props.db, shell.props.shell_player, True,False)

that entered into the rhythmbox console gives exactly the same errors.

EDIT:

step by step:

from the mailing list

entry_view = RB.EntryView.new(shell.props.db, shell.props.shell_player, True,False)

seems to create the object without errors.

It looks like you should never try to instantiate any RB objects via x = RB.Class()

this gives the signal errors.

Similarly I was doing:

qm = RB.RhythmDBQueryModel() to create a blank query model

This gave similar errors.

qm = RB.RhythmDBQueryModel.new_empty(self.shell.props.db) fixed this.

@asermax
Copy link
Contributor

asermax commented Sep 26, 2012

That's weird, I just tryied RB.EntryView( db=shell.props.db, shell_player=shell.props.shell_player ) on the Rhythmbox's python console and it worked without problems.
I think you only have to be carefull with how you pass the parameters. I used the Rhythmbox reference for python you posted a while ago to figure this out.

@fossfreedom
Copy link
Owner Author

excellent - another method to achieve the same :)

At the end-of-the day - looks like you have to manually create some RB objects such as EntryView - you cant create them via a UI file.

@fossfreedom
Copy link
Owner Author

@asermax - finished (hopefully) - if/when you have some time, please have a play and let me know what you think. TIA.

@asermax
Copy link
Contributor

asermax commented Sep 26, 2012

Nice, it looks much better now, and I think that making a separate module for the entry view was a wise decision, I hate to clutter all the code in one module xD
I made some little changes (just moved some code) to only show or hide the entryview when necesary (show it at startup if the option is enabled, show it immediately after enabling it and hide it immediately after disabling it), to only populate when it's enabled, and added the posiblity to clear the entry view when no album is selected.
A couple of issues we should tackle:

  • The lower pane size should be fixed to a reasonable size when clicking to expand it or make it customizable, saving the last used size (right now, when expanded it only occupies a very little portion of the viewport).
  • It would be nice that the entry view showed when a track is playing. For what I've seen so far, it doesn't have this capability yet.
  • Disable the GtkPaned's handle when the expander is not expanded or when the option to show entries is disabled.

After that, I think this feature would be ready to merge into master :3

Some optional features (more of a wishlist):

  • Have the same popup that shows on the library source popup on our entry view (except maybe the "Browse ..." variants).
  • Make the "sort by column" feature work.

PS: feel free to let me know if you don't agree with any of my changes :F

@fossfreedom
Copy link
Owner Author

ok - think I've solved the pane size, entry view playing state and the gtkpaned's handle. Give it a try.

The sort by column has baffled me - I think this is something to do with the querymodel but I dont really understand the sorting functions specified.

"The same popup as in the music library" - I think should probably be another enhancement issue - looking at the popup - maybe "wastebasket" and "playlist" stuff could equally apply to the icon-view as well. Not sure what "copy" does.

The Properties option looks like an RBSongInfo object - may have a look a little later.

@asermax
Copy link
Contributor

asermax commented Sep 26, 2012

Woot, great improvement. Now it feels more natural.
About the handle: it disables when the option is deactivated, but when the expander is collapsed, you still can move the handle, leaving a big space bellow the covers view. It's not that big deal, just a little detail I saw.

About using the same popup, I don't mean recreate it, but maybe figure out a way to reuse it (there may be a way to ask rhythmbox to show that same popup?). I don't think it aplies to the icon view since each icon represents an album, whereas in the entry view there are individual entries (and if we can reuse the popup, maybe we can reuse it's behavior). Maybe we can leave it as a research project for the future in another issue as you say.
When I've some time I'll see if I can figure out the sorting; I'm struggling with the source resources cleaning right now tho x_x

@fossfreedom
Copy link
Owner Author

I see what you mean by the expander when its collapsed - yes you can move the pane position.

I've had a dig around on the GtkPaned stuff - doesnt appear to be anything to "disable" the pane position. The pane position is disabled if we delete one of the child objects - which doesnt really make sense since you still need to see either the iconview or the GtkExpander.

I was looking also for a "signal" which I could tap into to recognise when the pane position has changed - I was hoping to "ping" the position back to the bottom if the expander was collapsed. Havent found one.

So, not really sure what we can do :(

Havent found an obvious function to "reuse" the library-source pop-up menu either.

I'll have a look at the cleaning stuff you've done so far tomorrow - will try to help.

@asermax
Copy link
Contributor

asermax commented Sep 27, 2012

Hey foss, I think the hidden expander hack is way to messy for the problem it solves. I found an alternate way to do the same whitout it, if you don't mind, I'll change the implmentation a lil bit.

@fossfreedom
Copy link
Owner Author

you are correct - its a hack ... hey if you have found another way - feel free to commit ;)

@fossfreedom fossfreedom reopened this Sep 27, 2012
asermax added a commit that referenced this issue Sep 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants