[GSoC] Advanced Library Filtering #1399

Merged
merged 29 commits into from Oct 9, 2012

Conversation

Projects
None yet
8 participants
Owner

Montellese commented Sep 9, 2012

Well this is it, the last PR from my GSoC 2012 work which will add the Advanced Library Filtering dialog to movie, tvshow, epsidode, musicvideo, artist, album and song views.

For team members: It's still with the "crappy" GUI integration. I've implemented the request to show the number of possible options for filters like genre, actors, directors etc next to the filter name so if there are 25 genres available to choose from for the current list of movies it will show "Genre [25]".

Member

arnova commented Sep 10, 2012

I've been waiting for this :-)

Member

ronie commented Sep 11, 2012

a few oddities i came across after using it for some time:

  • i have a hard time figuring out how to clear the filtered list and get a listing of all my movies back.
    i would expect hitting 'clear' and 'ok' in the dialog would do this, but it doesn't have any effect.
  • navigation is somewhat borked. when a filtered list is in effect, hitting back on my keyboard to navigate back to the home screen, we take this path:
    filtered list > root of movie lib > filtered list > root of video lib > home
  • slight skin breakage: the old filter button (id=19) doesn't show up anymore in other skins.
Owner

Montellese commented Sep 11, 2012

@ronie: Thanks a lot for taking the time to test this. You're the first person to provide any real feedback and I'm grateful.

  • Yup that seems to be borked because of some other changes I made in master to smartplaylists. I just fixed it locally. Will push the fix soon.
  • Hm I have discussed this with jmarshall and we decided that hitting back on a filtered list should remove the filter. Here is how it works for me when using the Backspace key: filtered list -> unfiltered list -> home screen. And when using the ESC key: filtered list -> home screen
  • That's because it isn't supposed to. XBMC decides for each view whether to show the old filter button or the advanced filter button so it's not really a breakage. I added the advanced filtering as a new button because it can be represented with an additional checkbox which indicates whether the current list is filtered or not. So skinners will have to add the additional button (id = 20) for the advanced filtering and XBMC will take care of which one to show and which one to hide. I don't really know anything about skinning so if that's not acceptable let me know.
Owner

Montellese commented Sep 11, 2012

Damn sent to early. Now I was able to reproduce your description of the navigation. If you filter a list, then press ESC to get back to the home screen and then go back into the list it goes like this:
filtered list -> ESC -> home screen -> Enter -> filtered list -> Back -> root of movie lib -> Back -> filtered list -> Back -> root of video lib -> Back -> Home

Very odd. I'll have to look into it. What would you expect to happen with the filtered list when you hit ESC? Should the filter still be applied next time you enter the same list or should it be discareded and show the full list again? I guess these kind of behaviours need to be discussed a bit more.

Owner

Montellese commented Sep 11, 2012

I pushed an update which fixes the "Clear" button not working and which also fixes the odd navigation issua ronie pointed out.

Owner

Montellese commented Sep 11, 2012

After talking to @ronie and @cptspiff about how to integrate the old filtering and the advanced filtering into skins we realized that we can't get away with using the same button for both the old and the advanced filtering. On request of cptspiff I've added 3 new info booleans on Container level: Container.CanFilter, Container.CanFilterAdvanced and Container.Filtered which can be used by skins in visibility conditions for the two filter buttons. Furthermore the advanced filtering functionality is no longer tied to a specific button/control ID but can be triggered using the new "filter" action (which will either bring up the dialog for the old or the advanced filtering).

Owner

Montellese commented Sep 13, 2012

And yet another update. I've known that staying on the currently selected item when changing a filter wasn't working because when changing the filter the path of every item changes (it always contains the current filter) but we use the path to identify the currently selected item. I added some code that should take care of this problem so when you filter (or clear a filter) it should just stay on the same item (unless the item doesn't match the filter anymore).

Owner

Montellese commented Sep 19, 2012

@jmarshallnz Would be grateful if you could take a look at the code as you were/are the mentor of my project and let me know if it's ok for October or not (obviously after a rebase).

jmarshallnz was assigned Sep 19, 2012

Member

jmarshallnz commented Sep 19, 2012

Assigned to me

Owner

Montellese commented Sep 20, 2012

Rebased onto latest master.

Owner

Montellese commented Sep 29, 2012

Once again rebased. Still waiting on final review and/or "good to go".

@jmarshallnz jmarshallnz commented on the diff Oct 1, 2012

xbmc/dialogs/GUIDialogMediaFilter.cpp
+
+ AddButton(filter.field, filter.label);
+ break;
+ }
+
+ case SettingInfo::RANGE:
+ {
+ float *valueLower = new float();
+ float *valueUpper = new float();
+ float min, interval, max;
+ RANGEFORMATFUNCTION format;
+ GetRange(filter, min, interval, max, format);
+
+ // don't create the filter if there's no real range
+ if (min == max)
+ break;
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

memleak of valueLower/Upper?

@Montellese

Montellese Oct 1, 2012

Owner

Yup, nice find.

@Montellese

Montellese Oct 1, 2012

Owner

Moved the variables down as they are not needed before this possible call to "break;" anyway

@jmarshallnz jmarshallnz commented on the diff Oct 1, 2012

xbmc/dialogs/GUIDialogMediaFilter.cpp
+ // set the heading label based on the media type
+ uint32_t localizedMediaId = 0;
+ if (m_mediaType == "movies")
+ localizedMediaId = 20342;
+ else if (m_mediaType == "tvshows")
+ localizedMediaId = 20343;
+ else if (m_mediaType == "episodes")
+ localizedMediaId = 20360;
+ else if (m_mediaType == "musicvideos")
+ localizedMediaId = 20389;
+ else if (m_mediaType == "artists")
+ localizedMediaId = 133;
+ else if (m_mediaType == "albums")
+ localizedMediaId = 132;
+ else if (m_mediaType == "songs")
+ localizedMediaId = 134;
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

could use a lut I guess (no big deal)

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Oct 1, 2012

xbmc/dialogs/GUIDialogMediaFilter.cpp
+ GetMinMax("songview", "iTimesPlayed", min, max);
+ }
+}
+
+bool CGUIDialogMediaFilter::GetMinMax(const CStdString &table, const CStdString &field, float &min, float &max, const CDatabase::Filter &filter /* = CDatabase::Filter() */)
+{
+ if (table.empty() || field.empty())
+ return false;
+
+ CDatabase *db = NULL;
+ CDbUrl *dbUrl;
+ if (m_mediaType == "movies" || m_mediaType == "tvshows" || m_mediaType == "episodes" || m_mediaType == "musicvideos")
+ {
+ CVideoDatabase *videodb = new CVideoDatabase();
+ if (!videodb->Open())
+ return false;
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

memleak, same below - could just use db = new CVideoDatabase(); and do the Open() below these blocks?

@Montellese

Montellese Oct 1, 2012

Owner

I'm not sure I remember correctly but I think I had that first but then changed it to this for some reason (which I obviously can't remember anymore).

@Montellese

Montellese Oct 1, 2012

Owner

Ah the compiler just reminded me why the call to Open() has to be done in there and on the CVideoDatabase/CMusicDatabase object: CDatabase::Open() is protected and can therefore not be called from outside.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Oct 1, 2012

xbmc/guilib/GUISliderControl.cpp
dirty |= nib.Process(currentTime);
-
- if (dirty)
- MarkDirtyRegion();
-
- CGUIControl::Process(currentTime, dirtyregions);
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

return dirty; here, and take care of it in Process() ?

@Montellese

Montellese Oct 1, 2012

Owner

Actually I just realized that it already works that way. dirty is a reference parameter passed to ProcessSelector() from Process() so Process() will get dirty == true if it is marked as such in ProcessSelector() and will also handle it there. Should I change that to passing dirty back as a return value instead of a reference parameter?

@jmarshallnz

jmarshallnz Oct 1, 2012

Member

Yeah, IMO that is better.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Oct 1, 2012

xbmc/guilib/GUISliderControl.cpp
break;
+
+ case ACTION_MOVE_DOWN:
+ // move to the upper slide if on lower slide
+ if (m_currentSelector == RangeSelectorLower)
+ {
+ SetRangeSelector(RangeSelectorUpper);
+ return true;
+ }
+ break;
+
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

I wonder if using SELECT to switch between the two would be more obvious?

@Montellese

Montellese Oct 1, 2012

Owner

I don't think I understand. You mean rename the SetRangeSelector() method? Or what do you mean with SELECT?

@jmarshallnz

jmarshallnz Oct 1, 2012

Member

I mean ACTION_SELECT to switch between the nibs in the slider.

@Montellese

Montellese Oct 1, 2012

Owner

Ah now I get it. Well I never considered it, not sure what would be more intuitive. But using SELECT instead of UP/DOWN would certainly make it easier to navigate through the list of filters because every UP/DOWN will bring you to a different filter instead of switching between the range selectors.

@Montellese

Montellese Oct 2, 2012

Owner

OK I implemented using ACTION_SELECT_ITEM instead of ACTION_MOVE_UP/DOWN so ppl can give it a try. For me it's hard to say which would be more intuitive as I've used the up/down implementation for a few months now and I'm very used to it. Using select certainly makes the general navigation through the list of filters more consistent as every up/down press brings you to another filter. But I don't know if I would have intuitively pressed Select/Enter to get to the other slider.

@jmarshallnz jmarshallnz commented on an outdated diff Oct 1, 2012

xbmc/guilib/GUISliderControl.cpp
@@ -190,56 +272,120 @@ void CGUISliderControl::SendClick()
}
}
-void CGUISliderControl::SetPercentage(int iPercent)
+void CGUISliderControl::SetRangeSelection(bool rangeSelection)
+{
+ if (m_rangeSelection == rangeSelection)
+ return;
+
+ m_rangeSelection = rangeSelection;
+ SetRangeSelector(RangeSelectorLower);
+}
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

This should potentially have a SetInvalid() on it - same with the one below.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Oct 1, 2012

xbmc/music/MusicDatabase.cpp
+ items.SetPath(musicUrl.ToString());
+
+ // cleanup
+ m_pDS->close();
+
+ return true;
+ }
+ catch (...)
+ {
+ m_pDS->close();
+ CLog::Log(LOGERROR, "%s failed", __FUNCTION__);
+ }
+
+ return false;
+}
+
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

Could these two routines share some base?

@Montellese

Montellese Oct 1, 2012

Owner

Probably yes. They mainly differ in the WHERE and GROUP clause (which is easily handled with the Filter class) and in the field read from the database and used for the label and path.

@Montellese

Montellese Oct 1, 2012

Owner

I added GetCommonNav which takes a table/view name and the name of the field in that table/view to be used as the label of the retrieved item. That safes quite some code duplication.

@jmarshallnz jmarshallnz commented on the diff Oct 1, 2012

xbmc/windows/GUIMediaWindow.cpp
return true;
- }
- if (GetProperty("filter").empty())
- {
- CStdString filter = GetProperty("filter").asString();
- CGUIKeyboardFactory::ShowAndGetFilter(filter, false);
- SetProperty("filter", filter);
- }
- else
- OnFilterItems("");
- return true;
+
+ return Filter();
}
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

This looks odd...

Ah, NM - didn't notice the function Filter() :)

@Montellese

Montellese Oct 1, 2012

Owner

Why? Filter() contains all the logic to update the button, show whatever dialog is required etc. I moved it there because I need it in ACTION_FILTER handling as well and didn't wanna duplicate the code.

@jmarshallnz jmarshallnz commented on the diff Oct 1, 2012

xbmc/windows/GUIMediaWindow.cpp
+ if (newOptions.HasOption("filter"))
+ {
+ CVariant filter;
+ if (newOptions.GetOption("filter", filter) && filter.isString())
+ curOptions.AddOption("filter", filter.asString());
+ }
+ else if (curOptions.HasOption("filter"))
+ curOptions.AddOption("filter", "");
+
+ string options = curOptions.GetOptionsString();
+ if (!options.empty())
+ curUrl.SetOptions("?" + options);
+ else
+ curUrl.SetOptions("");
+ currentItem = curUrl.Get();
+ }
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

This stuff seems really hacky, though I don't have a better suggestion atm, given that the paths are changing all the time.

@Montellese

Montellese Oct 1, 2012

Owner

Yeah I'd love to come up with a better implementation but I wasn't able to so far either :-/

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Oct 1, 2012

xbmc/windows/GUIMediaWindow.cpp
@@ -1576,10 +1626,10 @@ bool CGUIMediaWindow::GetFilteredItems(const CStdString &filter, CFileItemList &
// but it's re-enabled on the way back out.
CStdString match;
/* if (item->GetFocusedLayout())
- match = item->GetFocusedLayout()->GetAllText();
- else if (item->GetLayout())
- match = item->GetLayout()->GetAllText();
- else*/
+ match = item->GetFocusedLayout()->GetAllText();
+ else if (item->GetLayout())
+ match = item->GetLayout()->GetAllText();
+ else*/
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

cosmetic req'd ?

@Montellese

Montellese Oct 1, 2012

Owner

Yup will align properly.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Oct 1, 2012

xbmc/windows/GUIMediaWindow.cpp
+ items.SetPath(resultItems.GetPath());
+ return true;
+}
+
+bool CGUIMediaWindow::IsFiltered()
+{
+ return (!m_canFilterAdvanced && !GetProperty("filter").empty()) ||
+ (m_canFilterAdvanced && !m_filter.IsEmpty());
+}
+
+bool CGUIMediaWindow::Filter()
+{
+ // basic filtering
+ if (!m_canFilterAdvanced)
+ {
+ if (GetControl(CONTROL_BTN_FILTER)->GetControlType() == CGUIControl::GUICONTROL_EDIT)
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

ptr check?

@Montellese

Montellese Oct 1, 2012

Owner

Yup will add.

@jmarshallnz jmarshallnz commented on an outdated diff Oct 1, 2012

xbmc/windows/GUIMediaWindow.h
@@ -73,20 +77,31 @@ class CGUIMediaWindow : public CGUIWindow
void ClearFileItems();
virtual void SortItems(CFileItemList &items);
- /* \brief Called on response to a GUI_MSG_FILTER_ITEMS message
+ virtual bool CheckFilterAdvanced(CFileItemList &items) { return false; }
+ virtual bool Filter();
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

Maybe OnFilter() ?

@jmarshallnz jmarshallnz commented on an outdated diff Oct 1, 2012

xbmc/windows/GUIMediaWindow.h
Filters the current list with the given filter using FilterItems()
\param filter the filter to use.
\sa FilterItems
*/
void OnFilterItems(const CStdString &filter);
- /* \brief Retrieve the filtered item list
+ /* \brief Retrieve the filtered item list
@jmarshallnz

jmarshallnz Oct 1, 2012

Member

cosmetic not really required (similarly above)

Owner

Montellese commented Oct 3, 2012

OK I have squashed the fix commits into the original ones (except for the ACTION_SELECT_ITEM one) and rebased onto latest master.

There are two things left to do:

  1. Decide how to do the navigation between the two sliders/range selectors in the CGUISliderControl. The two options right now are either using Select/Enter/OK to switch between the two sliders which has the advantage of being able to get to the next/previous filter options without any extra clicks but requires an additional button, or to use the move up/down button to switch between the two sliders which has the disadvantage of requiring an extra button press to get to the next/previous filter option but doesn't need any extra buttons.
  2. Update Xcode project files for OSX.

Any opinions on what is more intuitive for the navigation within the slider/range selector? My initial implementation followed the second option i.e. doing everything with "move up/down" keys and I've used that for a while now so I can't really judge if using Select/Enter/OK is more intuitive/easier to use.

Member

da-anda commented Oct 3, 2012

as mentioned on IRC I gave the navigation/handling of sliders a thought and have another proposal on how navigating them could work. It's just a proposal and I'm also fine with the "press select to switch between controls" thingy.

Idea 1:

This one will only work if we would add checkboxes to enable/disable filters. Disabled filters could be greyed out and their options probably also hidden until they are enabled. Given this, up/down will by default scroll through the list as they always do. Once a slider filter is enabled, left/right can be used to move between checkbox and the two slider handles. When focused on a slider handle, up/down can be used to indrease/decrease the value (move the slider). To scroll up/down in the list again one has to focus the checkbox (unfocus the slider handles).

Idea 2:

This also works without the additional checkboxes. up/down always navigate the list. When focusing a slider item, the first handle is focused. Switching between the handles can be done with left/right, just as expected. To move a handle it has to be "activated" by pressing enter (status needs to be visually indicated) . Once "activated" left/right can be used to move the slider - up/down will still navigate the list. To "deactivate" and switch to the other handle one has to either press select or back and can then switch between the handles with left/right again. The benefit of this behavior would be that it's almost like draging with the mouse - so click'n'drag (select = mouse-down, left/right = drag, second select/back = mouse up).

Are just proposals/ideas - nothing more. Don't want to block this PR.

Member

jmarshallnz commented Oct 3, 2012

I wonder whether up/down to move a left/right slider is counter-intuitive? I think it would be OK if there was a clear "click to activate" where the slider nib very clearly came into focus. However, it's not obvious to me that a further click would deactivate the slider, whilst still keeping it in effect (I might be tempted to hit BACK instead?)

The click technique seemed natural enough when I first tried the UI, but as pointed out above - once you know, you know :)

Member

ronie commented Oct 6, 2012

this is gonna be a long shot....
if we want an intuitive way to control the range slider, without any navigation issues, it should be done like this:
http://i687.photobucket.com/albums/vv237/roniez/tmp/slider.png

that being said, i'm fine with the current implementation as well.

Owner

MartijnKaijser commented Oct 6, 2012

@ronie
looks good

Owner

Montellese commented Oct 7, 2012

@ronie While I think your idea makes it far more obvious how to use it, it requires more clicks in general to move the correct slider in the direction you want i.e. if you want to move the upper slider further up you first have to press 3 times to even be able to do that.

Montellese added some commits Jun 26, 2012

@Montellese Montellese videodb: make sure the items' path is correct 04ce920
@Montellese Montellese videodb: add possibility to only count results for GetPeopleNav, GetN…
…avCommon and GetTagsNav
2b18958
@Montellese Montellese CDatabase: add overloaded GetSingleValue() which takes a full query dc8abe0
@Montellese Montellese musicdb: adjust/extend SQL queries to better support WHERE clauses fo…
…r filtering
b00cd05
@Montellese Montellese musicdb: add GetAlbumTypesNav and GetMusicLabelsNav 2f3e1a3
@Montellese Montellese utils: add HasOption and GetOption to CUrlOptions cada025
@Montellese Montellese videodb: add an extra check for a "setid" option to GetMoviesByWhere 9fc3069
@Montellese Montellese CSmartPlaylistDirectory: add an optional parameter for the basedir to…
… GetDirectory()
dd7c497
@Montellese Montellese CSmartPlaylist: add Reset() and IsEmpty() be31b8e
@Montellese Montellese CSmartPlaylistRule: add OPERATOR_BETWEEN for range filters 3b33950
@Montellese Montellese videodb: add "filter" option to videodb:// URL 6017308
@Montellese Montellese musicdb: add "filter" option to musicdb:// URL cbfb997
@Montellese Montellese musicdb: make sure the items' path is correct 4431e45
@Montellese Montellese CSmartPlaylistDirectory: avoid variable name clash 3f0e86d
@Montellese Montellese CSmartPlaylist: improve genre rule for artists 86a8f57
@Montellese Montellese URIUtils: add support for filtered musicdb and videodb paths in GetPa…
…renPath()
df330ab
@Montellese Montellese CSmartPlaylistDirectory: make sure the "xps" option in the URL is alw…
…ays right (i.e. not present if the filter is empty)
d44891b
@Montellese Montellese videodb/musicdb: remove the "filter" and "xsp" options if they don't …
…match the currently retrieved item type
7ce8dd9
@Montellese Montellese CGUISliderControl: add support for range selection abc7773
@Montellese Montellese CGUIDialogSettings: add a RANGE type for settings 3476396
@Montellese Montellese add new dialog CGUIDialogMediaFilter 918f64b
@Montellese Montellese [win32] update project files 221eaf9
@Montellese Montellese GUIDialogMediaFilter: show the number of available choices for filter…
…s like genre, actors, studios etc
d1e8d38
@Montellese Montellese media library: add new logic for advanced filtering using a smartplay…
…list

There are three new Container info booleans for skins: Container.CanFilter,
Container.CanFilterAdvanced and Container.Filtered where CanFilter and
CanFilterAdvanced are mutually exclusive. These can be used for visibility
conditions of the existing "Filter" button and a new "Filter" button for
advanced filtering.
Furthermore a new action "filter" is available which will open the respective
filter dialog depending on whether the container/list supports simple or
advanced filtering.
b027242
@Montellese Montellese media library: adjust the paths and the history to be able to return …
…to a filtered list
9a1699b
@Montellese Montellese [confluence] add new button for advanced filtering 3d16eac
@Montellese Montellese media library: stay on the selected item during filtering 8f87a23
@Montellese Montellese [confluence] add DialogMediaFilter.xml c1d21ec
Owner

Montellese commented Oct 8, 2012

OK jmarshall and I have decided that we will use the Select/Enter/OK button to switch between the range sliders for now as it requires less special handling. I have squashed that commit into the CGUISliderControl commit and everything should be merge-ready now. Anything else to squash down?

@Voyager1 Voyager1 commented on the diff Oct 8, 2012

xbmc/windows/GUIMediaWindow.cpp
@@ -1593,7 +1643,112 @@ bool CGUIMediaWindow::GetFilteredItems(const CStdString &filter, CFileItemList &
items.ClearItems();
items.Append(filteredItems);
- return (items.GetObjectCount() > 0);
+
+ return items.GetObjectCount() > 0;
+}
+
+bool CGUIMediaWindow::GetAdvanceFilteredItems(CFileItemList &items, bool &hasNewItems)
+{
+ hasNewItems = false;
+
+ CFileItemList resultItems;
@Voyager1

Voyager1 Oct 8, 2012

Member

you should probably initialize the path of the list with m_vecItems->GetPath() here, since you're assigning that at the end to "items".
See Pull request 1176 (#1176) where tvshows/seasons watched-filtering is using the list path to determine the node type.

@Montellese

Montellese Oct 8, 2012

Owner

That's done in the database anyway (there are two commits for it, one for videodb and one for musicdb) because otherwise the filtering in the filter dialog wouldn't work properly either. But I can added, doesn't really make any difference.

@Voyager1

Voyager1 Oct 8, 2012

Member

you're right, I didn't notice that addition.

Montellese was assigned Oct 9, 2012

@Montellese Montellese added a commit that referenced this pull request Oct 9, 2012

@Montellese Montellese Merge pull request #1399 from Montellese/gsoc_advanced_filter
[GSoC] Advanced Library Filtering
39ab944

@Montellese Montellese merged commit 39ab944 into xbmc:master Oct 9, 2012

Owner

MartijnKaijser commented Oct 9, 2012

Currently de slider for addon settings is broken
http://imageshack.us/a/img823/3310/screenshot004hh.png

Owner

Montellese commented Oct 10, 2012

Looks alright to me :-P Thanks for the report, I'll look into it.

Owner

Montellese commented Oct 10, 2012

I checked all the addons I have installed (very few) and I only found two settings using a slider, both from screensavers (with a percentage value) and they work perfectly fine for me. Could you provide me with whatever addon shows that problem for you?

Member

Voyager1 commented Oct 10, 2012

something's wrong with this. When navigating to "recently added" the list first loads with the recently added items. The GetFilteredItems then overrides that by loading the complete movie list. Funny thing is that the real "recent" items get their thumb loaded - the others stay blank. The issue seems to be that the XPlaylist is not taking into account that this was just the "recently added" list, but thinks it's the entire movie list.

Owner

Montellese commented Oct 10, 2012

Yeah I'm aware of the issue and have already fixed it locally. Still testing if there's any other odd behaviour because of the fix.

I suggest to add a database filter here to check here minimum year to be > 0. I had an issue with movies that were manually added without a year. It's very difficult to use the slider to go from 0 to 2012!

e.g. filter.where = year + " > 0";
you'll also have to define it and include it below as the last param to GetMinMax(table, year, min, max, filter);

Owner

Montellese replied Oct 10, 2012

Hm I thought I added that functionality because I also have movies with no year set and they don't show up for me. Will look into it.

Tested my proposed modification to the dialog to filter on "year". If you clear the filter or even leave the min boundary untouched on the slider, even though the minimum might be let's say 1944 when excluding 0, you still get the result back for those movies that have year = 0. I tried it here on my DB (I have 2 movies with year = 0).
edit: not sure I'm making sense... (it's late) but I think that is acceptable behavior, and makes the slidr usable (it's silly having to slide the minimum all the way from zero to 19xx).

Owner

Montellese replied Oct 11, 2012

See 2fc6f76. The reason why movies with year == 0 still show up if you don't touch the range selection is because the logic first checks if any of the sliders have been moved and only applies the > and < rules if the respective slider has been moved. But IMO it's better that these movies show up as well if the minimum year slider hasn't been moved because otherwise they wouldn't show up at all.

Thanks & I agree. Movies with year==0 need to show up (if not filtering on year). However slider needs to start at smallest non-zero value. Your commit seems to do exactly that.

Member

Voyager1 commented Oct 10, 2012

Montellese, there's still something funky going on with the set thumbs. Especially, when you enter a set, not all thumbs get loaded, and then get back out of it, the set thumb is gone. So are the other sets... When you move the cursor, thumbs appear one at a time.

Member

Voyager1 commented Oct 10, 2012

Montellese, another issue: when you change the setting "group movies in sets", it only takes effect on the movie list when restarting xbmc. That used to work before.

Owner

Montellese commented Oct 11, 2012

I have to look onto the thumb loading (again).
Concerning toggling Group in Movie sets this has been a bug in mainline ever since that feature has been added but I only noticed two weeks ago. Tbh I forgot about it. The problem is that we don't reset the cache for movie lists when that setting is toggled.

Owner

Montellese commented Oct 11, 2012

Fixed the artwork problem (see cadede2) although it had nothing to do with this PR but with @jmarshallnz artwork changes. Don't go blaming everything on me :-P

Owner

Montellese commented Oct 11, 2012

And 05593e4 takes care of the issue when changing the "Group movies in sets" setting.

Member

Voyager1 commented Oct 11, 2012

thanks so much for both fixes and apologies for the 'blame' - I was so focused on testing the filtering update that I forgot about the other major changes that had gone in.

@Montellese
I am trying to add a helper comment for translators, but I cannot find where we actually use this string (#21466).
Could you please help ?
Thanks!

Owner

Montellese replied Oct 15, 2012

Nice find, I first used this one instead of the ones below (#21467 - #21469) but then found that those work better. So this one isn't used anymore and can be removed.

kib referenced this pull request in kib/skin.neon Jan 17, 2013

Open

[Frodo] Support DialogMediaFilter.xml, new in Frodo #3

Montellese deleted the Montellese:gsoc_advanced_filter branch Apr 4, 2014

@tru tru added a commit to plexinc/plex-home-theater-public that referenced this pull request Nov 27, 2014

@tru tru Compare connection flags properly.
This caused some restricted connections to be dropped since a
connection could have both CONNECTION_MYPLEX and CONNECTION_DISCOVERED
set. Now we just check if we have token for the connection instead, it
should give the same result, hiding discovered servers that doesn’t
have a mapping.

Related to #1399
3a9fb03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment