Skip to content

[rar] add a callback class for unrarlib rather than calling into guilib #5000

Merged
merged 2 commits into from Jul 14, 2014

4 participants

@jmarshallnz
Team Kodi member

We don't want to call into guilib directly from unrarlib, so this does the same as what we were doing (though with a 200ms delay) via a callback class.

@mkortstiege This should be pretty equivalent to what we already have in master. i.e. it is useful as a base from which to make further changes, such as increasing the time before display, or switching to busy dialog etc.

@mkortstiege
Team Kodi member

Yep, that will do. Thanks.

@jmarshallnz
Team Kodi member

Oh, needs testing ofc - I didn't do any at all :p

@mkortstiege
Team Kodi member

OK, will give it a chance around the weekend.

@mkortstiege
Team Kodi member

Looks good and works for me, thanks. Not sure if 200 ms is enough for slower hardware though or if we should make it adjustable to get an consistent UX. @popcornmix what's again the timings for rar stuff on the PI?

@popcornmix
Team Kodi member

Doesn't seem to calculate percentage correctly.
Tried two files and one pops up "-6%" (yes, negative) and the other "-31%" and the progress bar immediately disappears.

@jmarshallnz
Team Kodi member

My bet is it did it before this change as well? Can't see how the percentage is calculated any differently here at least?

@popcornmix
Team Kodi member

I was wrong about the negative, I think it's just a "-" used as a separator.

Without this PR the progress bar stays up for many seconds and updates from 0 to 100%.
With this PR the progress bar just flicks up for a fraction of a second.

Looks like this PR makes the rar extraction fail:

11:35:46 55235.421875 T:2901001296   ERROR: filerar::open failed to cache file The.Wolf.of.Wall.Street.2013.720p.BluRay.X264-AMIABLE.sub

Log without this PR: http://paste.ubuntu.com/7784423/
Log with this PR: http://paste.ubuntu.com/7784438/

@mkortstiege
Team Kodi member

That's weird. I have no such filerar::open issues here. All external (rarred) subs are detected properly.

@jmarshallnz jmarshallnz commented on an outdated diff Jul 12, 2014
xbmc/filesystem/RarManager.cpp
+ progress_info(const std::string &file) : heading(file), shown(false), showTime(200) // 200ms to show...
+ {
+ }
+ ~progress_info()
+ {
+ if (shown)
+ {
+ // close progress dialog
+ CGUIDialogProgress* dlg = (CGUIDialogProgress*)g_windowManager.GetWindow(WINDOW_DIALOG_PROGRESS);
+ if (dlg)
+ dlg->Close();
+ }
+ }
+ bool progress(int progress, const char *text)
+ {
+ bool cancel(true);
@jmarshallnz
Team Kodi member
jmarshallnz added a note Jul 12, 2014

heh, we probably want cancel(false); here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz
Team Kodi member

Defaulting to cancel = false is probably the reason for the failures :)

@jmarshallnz
Team Kodi member

Updated - should be all good now. @popcornmix mind testing when you have a few?

@jmarshallnz jmarshallnz added the Helix label Jul 13, 2014
@popcornmix
Team Kodi member

Fails again.
The logic for cancel is inverted.
You fail if:

    if (!m_progress(m_context, int(float(CurUnpWrite)/float(((Archive*)SrcFile)->NewLhd.FullUnpSize)*100), NULL))

But progress returns true if cancelled.

Tested with progress returning "!cancel" and it behaves as desired.

@jmarshallnz
Team Kodi member

Ah, I switched the wrong bool. I'll add some doxy about what it's supposed to do then make sure it does it. While paying attention this time :)

@jmarshallnz
Team Kodi member

@popcornmix, @mkortstiege updated - pretty confident this time (3rd, 4th times the charm?)

Jonathan Mar... added some commits Jun 9, 2014
Jonathan Marshall [rar] add a callback class for unrarlib rather than calling into guil…
…ib direct from UnrarXLib
2fcd1b4
Jonathan Marshall [progress] make CGUIDialogProgressBar::ShowProgressBar/SetCanCancel t…
…hread-safe without messing about with graphicscontext
5e0ffdb
@jmarshallnz
Team Kodi member

rebased. jenkins build this please

@mkortstiege
Team Kodi member

Works. Jenkins build failure is unrelated.. Android slave has problems as it seems.

@popcornmix
Team Kodi member

Looks okay to me.

@jmarshallnz jmarshallnz merged commit 551a08a into xbmc:master Jul 14, 2014

1 check failed

Details default Merged build #1024 failed in 25 min
@jmarshallnz jmarshallnz deleted the jmarshallnz:hacky_rar_callback branch Jul 14, 2014
@MartijnKaijser MartijnKaijser added this to the Helix 14.0-alpha1 milestone Jul 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.