nightingale-media-player / nightingale-hacking

Working tree for the community fork of Songbird, Nightingale. If building, use the sb-trunk-oldxul (development) branch, with the tag 1.12.1 tag for stable, for now. The master-xul-9.0.1 branch is the current progress in building Nightingale with XULRunner 9 and builds, but is broken. All help in terms of patches and pull requests is welcome.
https://getnightingale.com
GNU General Public License v2.0
185 stars 41 forks source link

Enhance Metadata Editing Dialog #225

Closed rsjtdrjgfuzkfg closed 10 years ago

rsjtdrjgfuzkfg commented 11 years ago

Forum user rsunderl:

While you can mass edit (by selecting several files) the regular Metadata of files, it would be nice to also be able to mass edit the Nightingale-specific Metadata as well.

For instance, Keywords allow you to organize the media by such user-defined categories as "Mellow", "Upbeat", etc. and then use those keywords to generate Smart Playlists based on those keywords.

However, if you already have an extensive library of tunes, you currently have to edit the Keywords in a file-by-file manner.

With an extensive library, this could take weeks or even months.

By providing an additional tab in the "View Metadata" dialog, you could allow your users to mass edit the Nightingale-specific metadata more efficiently.

For the implementation, either add tabs / some other clean UI for it or do it as add-on overlaying the dialog. I'd prefer doing it in core, but open for discussion. Probably a good starter bug if someone is interested to get to know the NG core...

Forum user simon:

set the preference :

songbird.trackeditor.enableAdvancedTab

to true - you will get another tab in the metadata editor dialog which gives access to many additional media item properties, including keywords.

Seems like there is a built-in enumeration of all tags triggered by that. While it could be a great starting point, it does not seem end-user friendly to me...

luisegallego commented 10 years ago

In the advanced Tab here is a list of all the fields that I feel need to be removed since they are read only:

For some reason BPM is set as editable in metadata editor, but not in the advanced tab.

As for the implementation I would say just add a toggle box to "show/hide advanced settings" then if it is true somehow add these extra fields to the already existing window:

freaktechnik commented 10 years ago

show name, episode number and season episode are for videos and are shown by default in their metadata editor. I think we should't expose these on audio.

luisegallego commented 10 years ago

@freaktechnik , I thought the same thing at first but we have to consider podcasts. I don't listen to them anymore, but in the past I remember some come in .mp3 format.

freaktechnik commented 10 years ago

True. And that's why we need more mediatypes (audiobooks and podcasts).

luisegallego commented 10 years ago

I'm also thinking of hiding the "descriptions" tag field, since to me it seems to serve the same purpose as the "Comments" tag field.

freaktechnik commented 10 years ago

I'm curious as to from which tag format the "Description" field comes. Comment is from ID3 and is for all the misc. information. Maybe those two fields should even be merged, however I have not looked trough all metatag specifications taglib/ngale uses.

luisegallego commented 10 years ago

Hey guys just a quick update: I was able to clean up the already existing advanced Tab like we have discussed and removed all the fields that the user cannot edit. I also added what I think are called widgets, inside of the XUL file, containing all of all the advanced tags. I have been successful in displaying them properly (labels and textboxes) inside of the already existing metadata editor. Right now I'm working on having these tags(labels and textboxes) only pop up when the advancedTab preference is enabled or set to TRUE.

With that being said, do we want this implemenetation of the advance fields? Or do we want to keep them seperate in another tab like it already exists? If so, then my work I would say is pretty much done, I would only have to go back in and add an option for the advanceTab preference to be toggeled on/off from somewhere inside NG's preference menu, or even perhaps add a checkbox inside of the already existing metadata editor to accomplish this for us.

I can take pictures if it makes things easier! Thanks again guys!

Merry Christmas!!!

freaktechnik commented 10 years ago

I would appreciate a screenshot or two... I think the pref should onlly be exposed in the settings and I also think that we should avoid to add more content to the main metadata editor tab.

luisegallego commented 10 years ago

Here is what the window looks like: screenshot - 12252013 - 09 44 53 am

luisegallego commented 10 years ago

screenshot - 12302013 - 07 38 17 pm And screenshot - 12302013 - 07 38 29 pm

Here is what I might be looking to submit, just need your approvals.

https://github.com/luisgmarine/nightingale-hacking/tree/enhancedMetadataEditorV2

freaktechnik commented 10 years ago

I object the idea of there being a checkbox in the dialog to show another tab. Why wouldn't we just show the tab by default then, taht'd take WAY less space and ismuch easier to figure out for a new user. (thanks @Luisgmarine for all the work, though!)

Ezekiel000 commented 10 years ago

Wouldn't it be better to add the tickbox to the settings dialogue?

On 31/12/2013 11:09 AM, nightingale-media-player/nightingale-hackingreply@reply.github.com wrote:

I object the idea of there being a checkbox in the dialog to show another tab. Why wouldn't we just show the tab by default then, taht'd take WAY less space and ismuch easier to figure out for a new user.


Reply to this email directly or view it on GitHub: https://github.com/nightingale-media-player/nightingale-hacking/issues/225#issuecomment-31391190 Sent from my PlayStation®Vita system

freaktechnik commented 10 years ago

@Ezekiel000 -> http://irclogs.jackgrigg.com/irc.mozilla.org/nightingale/2013-12-25

luisegallego commented 10 years ago

If in turn we do take the path of adding this as a toggle option in the preference dialogue, there is already another property that relates to meta data. The ratings meta data.

screenshot - 12312013 - 05 48 45 am

Again this option is under "playback" , and I don't think that this would be the ideal place to put something forth that relates to toggling on/off the advance tags option.

The other option would be to place it under the "Advance" tab which makes a lot more logical sense. Once inside the advance tab I'm not sure where would be the best place to squeeze in a checkbox that does this toggling for us. Perhaps create even another tab that says " Metadata" and pull everything from the preference menu that is metadata related and display it in this new window.

@freaktechnik , no problem either, just glad to be helping out!

rsjtdrjgfuzkfg commented 10 years ago

I still like the idea of having the checkbox in the metadata dialog. However the current position is not nice, as it visually blocks the way more important controls for actual metadata. Not sure how it would look on the bottom. In general, I thought more of expanding the first tab than of adding a second tab. I agree with @freaktechnik that showing/hiding another tab doesn't seem intiutive; seeing how it looks I think it might even be sufficient not to provide a preference for this? Open for suggestions.

@luisgmarine thanks for your work!

luisegallego commented 10 years ago

Here is what it would look like with the checkbox at the bottom:

screenshot - 12312013 - 07 04 08 am

Here is what it looks like without the checkbox and the advance tags tab enabled:

blob

I used to have the xul file where I just integrated the four advance tags into the already existing window. If you would like I can try to write it up again. Overall it did make the window extend to the point where the scroll bars were enabled (@1920x1080 res) in the metadata window.

If I would vote for one, I would vote for the one that is already enabled by default and there is no option to hide/unhide the advance tag tab. This in my opinion is a win/win situation. If you want to use it, it's there, if not, having the tab show up does not pull away from the metadata editor window at all.

freaktechnik commented 10 years ago

As already said, I think just showing the tab by default without pref is the way to go. What happens to the metadata dialog for videos? Nothing, as the additional metadata fields are already in general there?

luisegallego commented 10 years ago

Here is what happens to the metadata dialog for videos:

screenshot - 01012014 - 07 09 57 am

Few things, for one, at this point we should look into either hiding the advance tab or removing it since the fields will be duplicated. The other thing is that we should also clean up the already existing layout. To me it looks a bit sloppy and could use a different set of alignment.

freaktechnik commented 10 years ago

I think we should hide the advanced tab on videos. But that should be quite easy to do, since it already has another properties tab, so the detection logic is somewhere in there. I don't care about the video's properties tab, so feel free to mess with it.

luisegallego commented 10 years ago

I'm not sure how to achieve this, I tried copying an example that declared a variable called isVideo that could determine the current type of media that is selected. https://github.com/luisgmarine/nightingale-hacking/commit/232ef3f0cab9e7224ca92ecc8879de9fab32b018

freaktechnik commented 10 years ago

What if you remove the advanced tab from trackEditorVideo.xul?

luisegallego commented 10 years ago

While I didn't edit the trackEditorVideo.xul, I found another way just editing the TrackEditor.xul file. Thanks for the help @freaktechnik , got me on the right path! I think this commit is ready, what say ye?

https://github.com/luisgmarine/nightingale-hacking/tree/enhancedMetadataEditorV2

rsjtdrjgfuzkfg commented 10 years ago

@luisgmarine I have not tested it; from roughly looking at it:

trackeditor.advanceTab.label typo? In general, please justify introducing a new locale string (as there is already trackeditor.tab.advanced in songbird.properties). I'm not sure what guideline we have for keeping duplicate strings in dtd and properties; maybe @GeekShadow knows the best practices there? If we're accepting the new string, we can't merge it in for 1.12.1 imho, as there is not enough time for translators.

Why do you keep the comment FIXME: hid summary and lyrics tabs halfheartedly?

Code style: I'd prefer keeping the advanced tag interface capable of displaying any list of tags (for example via constructor); this would also allow us of making the tags actually listed customizeable for power users and add-ons, and to introduce advanced tags for video quite easily in case we need them. In that case, keeping the readonly stuff would be required, too.

Commit style: you have tons of commits for not that much feature. While that's ok, I'd prefer to have less commits in future contributions, it's easier to read the changelog then.

luisegallego commented 10 years ago

back to the drawing board ...

AntoineTurmel commented 10 years ago

@luisgmarine @rsjtdrjgfuzkfg Hmm since it's a string for a XUL file, it has to be in .dtd so it's a new string for a new context. The string ID should be trackeditor.tab.advanced.title (makes senses if you look on edit/lyrics string ID) For the translation of the string it should be fairly easy since it's the same as trackeditor.tab.advanced (I think I can copy/paste it everywhere).

On multiple commits: You can stash commits with Git : http://git-scm.com/book/en/Git-Tools-Stashing

luisegallego commented 10 years ago

@AntoineTurmel , thank you for that tip. I honestly thought that I was getting away with squashing the commits. I knew about stashing, but in my mind I always though of it as something you do when switching from working branch to working branch.

To be honest I would have recreated another branch with just one clean commit to merge into NG. This was more for me to just learn and get you guys to criticize. I was hoping at the end to squash all these commits into a single big commit, but I think I might be understanding the squashing wrong.

@rsjtdrjgfuzkfg, yes that is a new string, and no it is not a typo. I should have taken into account the fact of translating, but at the same time I didn't know that there was a goal to put this into 1.12.1, if it was I would have worked harder!!!!

As for what we want I'm still drawing blanks and it is a bit hazy. So what we want is a constructor that will build the advance tab, where the user can pick and choose what properties are being displayed in the advanced tab. For example when I think of this I'm automatically thinking of checkboxes/dropdown box to display all tags or select which tags you want to see. Sort of how you pick what columns get displayed in the main media browser window ( like track/title/time/size/etc).

Please let me know what you guys think, I really want to get this through for you guys! Sorry I'm slow and sort of suck at coding, but I'm here to learn, I promise!

rsjtdrjgfuzkfg commented 10 years ago

@luisgmarine: the typo thing: didn't you want it to be advancedTab with a d?

About the code style stuff: While allowing the user to costomize it would be nice, having the constructor in code would allow us to easily add that feature later. It would make the tab more flexible. Of course you could implement the tag-selection as customizeable feature right away, but that would be quite a bit more work.

And there is no reason for you to be sorry about anything, we're glad you're here :)