Skip to content
This repository

Plugins drop progress dialog #1281

Merged
merged 7 commits into from over 1 year ago

4 participants

jmarshallnz Michal Piechowiak vicbitter Peter Frühberger
jmarshallnz
Owner

There's currently an issue with layered dialogs where a plugin throws up a keyboard dialog for search and the like. The dialogs involved are:

  1. Busy dialog from CDirectory::GetDirectory.
  2. Progress dialog from CPluginDirectory::WaitForScriptResult.
  3. Keyboard dialog from plugin.

The problem case is where the progress pops up after the keyboard dialog is on screen, which often happens in plugins which implement a search feature, such as youtube.

This fixes it by eliminating the progress dialog**. This isn't ideal, as we lose the progress from the plugin (such as number of items to retrieve, and progress retrieving them). I'm not sure whether or not many plugins use this or not. IMO extending busy would be a nicer way to handle this either way - plugins that take a lot of time can throw up the progress themselves should they wish to (some already do). One disadvantage with this is that it's not necessarily obvious that it can be cancelled perhaps?

An alternate fix would be to stop the progress popping up if another modal dialog pops up. Trouble with this is it's hard to determine this case nicely inside the PluginDirectory class, as the app loop is running at this point, and thus there's potential races during tests for modal dialogs, as well as the busy gets in the way.

Yet another alternate might be that the progress stuff is moved into CDirectory::GetDirectory if the directory class can be cancelled.

Comments/criticisms welcome.

**Note that we don't have the same issues with busy being on screen, as it's automatically hidden when another dialog pops up over the top of it, and also the keyboard dialog can't pop up until it's on screen, which guarantees the keyboard is on top, thus receives input.

Deleted user

The content you are editing has changed. Reload the page and try again.

i see no way around this. question becomes if we should allow some backwards compat here (spec'd in addon.xml). i.e. anything running under py 2.0/1.0 will get their dialog unless specified? bit counter-productive as the bug may very well bite in those plugins, but still this is a rather harsh behaviour change...

Sending Request…

Attach images by dragging & dropping or selecting them. Octocat-spinner-32 Uploading your images… Unfortunately, we don't support that file type. Try again with a PNG, GIF, or JPG. Yowza, that's a big file. Try again with an image file smaller than 10MB. This browser doesn't support image attachments. We recommend updating to the latest Internet Explorer, Google Chrome, or Firefox. Something went really wrong, and we can't process that image. Try again.

Michal Piechowiak
Collaborator
pieh commented August 14, 2012

How about something like this pieh@247cdd5 ? (messy as this is just draft). It re-adds progress dialog that runs instead of busy dialog in CDirectory::GetDirectory on gui thread. Most important part are changes to IDirectory (did my best to comment them doxy style in IDirectory.h but I defenitely suck at doing that ;P)

Note: this patch is addition to commits in this PR - not usable in master

This doesn't solve plugins trying to spit their own progress dialog, dunno if we need to fix it just here or globaly (active GUIWindow being taken over by something else)

jmarshallnz
Owner

Given that the only info we're displaying is the progress percentage, the interface could be simpler to just include that percentage, and then you could drop the lock. Also, we could potentially hook it up to the busy dialog, which is possibly a bit lighter, and doesn't have the issue with clashing with the progress. I think the existing busy could just have a percentage added to it (progress or label) easily enough without breaking skins.

@Jezz_X your thoughts?

Michal Piechowiak
Collaborator
pieh commented August 14, 2012

Currently we're also displaying script name in heading, "Loading directory" in first line and "Retrieved %i items" in last line, but I guess we can just drop them as they're not essential.

That struct was meant to be used generally with GUIDialogProgress so that's why I stuffed percentage,heading and lines there and placed it in IProgressCallback.h.

And as I said, this was just a draft (now i even see that I forgot to update percentage, but still updating line with items loaded count) - my main idea was to pass progress info from IDirectory to CDirectory::GetDirectory(). What dialog, how much progress info to pass? That's up to You guys.

jmarshallnz
Owner

I like the idea of allowing feedback, but IMO the busy would work better than the main dialog overall:

Advantages:

  1. Much lighter weight.
  2. Doesn't have issue with progresses that python or other VFS's might throw up.
  3. Doesn't require progress should the VFS not provide it.
  4. Simpler code (no locking required on the progress, a single new (const) function to IDirectory to get progress).

Disadvantages:

  1. Skin change required (though it would be backcompat)
  2. Not as obvious that the process is cancellable?

I suspect number 2 isn't much of a problem - the natural thing when something is busy is to try going back? If we find it's an issue, a label could be added to the working dialog to make it clear.

Michal Piechowiak
Collaborator
pieh commented August 15, 2012

DialogBusy now is also cancalable (if we cancel, IDirectory continue to fetch dir, but we don't wait on it and don't care about items that will eventually be fetched), so I would remove 2. from disadvantages (or rather say it's not directly related to this). /agree with rest.

jmarshallnz
Owner

I meant not as obvious to the user (no gigantic "Cancel" button).

I'll mock something up on the weekend unless someone beats me to it.

vicbitter

Guys, I think commit 49cc4be is causing the problem. If you comment out the commit then the "Loading Directory" dialog is not shown over the top of the Search dialog.

vicbitter

@jmarshallnz, I tried your commits with the "lock.leave()" commit and everything worked great. Thanks

jmarshallnz
Owner

Ok, some work on progress bar to busy dialog can be found here (branched off here).

https://github.com/jmarshallnz/xbmc/tree/add_progress_to_busy_dialog

@Jezz_X: nasty skinning work there by me - would appreciate you having a crack. ATM it's id 10 - not sure whether to make it an infolabel or not - your thoughts?

jmarshallnz
Owner

@JezzX: see above.

Jonathan Mar... added some commits August 12, 2012
jmarshallnz

@JezzX I've pulled in the confluence progress dialog addition to busy. Would appreciate you taking a look. ATM it's a fixed ID, but I could do an infolabel if you think that is more useful.

This is mostly a fix, but as it changes API, it would be nice to get this in the merge window.

Peter Frühberger
Collaborator

@jmarshallnz:
Could you rebase it on the latest master code. We want to test it within our xvba ppa. Disappearing plugin dialogs is a great issue there.

Deleted user

The content you are editing has changed. Reload the page and try again.

button is green.......

Sending Request…

Attach images by dragging & dropping or selecting them. Octocat-spinner-32 Uploading your images… Unfortunately, we don't support that file type. Try again with a PNG, GIF, or JPG. Yowza, that's a big file. Try again with an image file smaller than 10MB. This browser doesn't support image attachments. We recommend updating to the latest Internet Explorer, Google Chrome, or Firefox. Something went really wrong, and we can't process that image. Try again.

jmarshallnz

@cptspiff: what do you think about merging as a fix (there's still issues with search dialogs popping up under the progress in current master)

Obviously the skin needs work, but I can drop 7285c2e and ask @JezzX to do it (or do it myself and thus force the issue)

Deleted user

The content you are editing has changed. Reload the page and try again.

it is a fix. and it works fine i have had it in my builds for a while. green button is up for some hitting.

Sending Request…

Attach images by dragging & dropping or selecting them. Octocat-spinner-32 Uploading your images… Unfortunately, we don't support that file type. Try again with a PNG, GIF, or JPG. Yowza, that's a big file. Try again with an image file smaller than 10MB. This browser doesn't support image attachments. We recommend updating to the latest Internet Explorer, Google Chrome, or Firefox. Something went really wrong, and we can't process that image. Try again.

jmarshallnz jmarshallnz merged commit b9d289d into from September 17, 2012
jmarshallnz jmarshallnz closed this September 17, 2012
kibje kibje referenced this pull request in kibje/skin.neon January 17, 2013
Open

Replace custom progressbar with new Frodo builtin #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 7 unique commits by 1 author.

Sep 07, 2012
adds prototype for cancelling directory progress from CDirectory::Get…
…Directory
0a249b4
drop the progress dialog from plugins retrieving folders, preferring …
…to use the busy dialog from CDirectory::GetDirectory. Fixes #13244
834e3b5
cosmetics: indenting 75fd3a5
adds GetProgress to IDirectory, and implement for CPluginDirectory 037fcd5
adds progress dialog support (id 10) to the busy dialog, and utilise …
…in CDirectory::GetDirectory
00ef83c
cleanup - no need for updating labels on the progress during plugin d…
…irectory fetch, as it is using busy dialog now
6aad20c
[confluence] adds progress control to busy dialog 7285c2e
This page is out of date. Refresh to see the latest.
10  addons/skin.confluence/720p/DialogBusy.xml
@@ -39,6 +39,14 @@
39 39
 				<label>$LOCALIZE[31004]</label>
40 40
 				<font>font12</font>
41 41
 			</control>
  42
+			<control type="progress" id="10">
  43
+				<description>Progressbar</description>
  44
+				<posx>20</posx>
  45
+				<posy>65</posy>
  46
+				<width>160</width>
  47
+				<height>10</height>
  48
+				<!-- <info>System.Progressbar</info> -->
  49
+			</control>
42 50
 		</control>
43 51
 	</controls>
44  
-</window>
  52
+</window>
20  xbmc/dialogs/GUIDialogBusy.cpp
@@ -20,13 +20,17 @@
20 20
  */
21 21
 
22 22
 #include "GUIDialogBusy.h"
  23
+#include "guilib/GUIProgressControl.h"
23 24
 #include "guilib/GUIWindowManager.h"
24 25
 
  26
+#define PROGRESS_CONTROL 10
  27
+
25 28
 CGUIDialogBusy::CGUIDialogBusy(void)
26 29
   : CGUIDialog(WINDOW_DIALOG_BUSY, "DialogBusy.xml"), m_bLastVisible(false)
27 30
 {
28 31
   m_loadOnDemand = false;
29 32
   m_bModal = true;
  33
+  m_progress = 0;
30 34
 }
31 35
 
32 36
 CGUIDialogBusy::~CGUIDialogBusy(void)
@@ -40,6 +44,7 @@ void CGUIDialogBusy::Show_Internal()
40 44
   m_bModal = true;
41 45
   m_bLastVisible = true;
42 46
   m_closing = false;
  47
+  m_progress = 0;
43 48
   g_windowManager.RouteToWindow(this);
44 49
 
45 50
   // active this window...
@@ -53,6 +58,16 @@ void CGUIDialogBusy::DoProcess(unsigned int currentTime, CDirtyRegionList &dirty
53 58
   if(!visible && m_bLastVisible)
54 59
     dirtyregions.push_back(m_renderRegion);
55 60
   m_bLastVisible = visible;
  61
+
  62
+  // update the progress control if available
  63
+  const CGUIControl *control = GetControl(PROGRESS_CONTROL);
  64
+  if (control && control->GetControlType() == CGUIControl::GUICONTROL_PROGRESS)
  65
+  {
  66
+    CGUIProgressControl *progress = (CGUIProgressControl *)control;
  67
+    progress->SetPercentage(m_progress);
  68
+    progress->SetVisible(m_progress > 0);
  69
+  }
  70
+
56 71
   CGUIDialog::DoProcess(currentTime, dirtyregions);
57 72
 }
58 73
 
@@ -68,3 +83,8 @@ bool CGUIDialogBusy::OnBack(int actionID)
68 83
   m_bCanceled = true;
69 84
   return true;
70 85
 }
  86
+
  87
+void CGUIDialogBusy::SetProgress(float percent)
  88
+{
  89
+  m_progress = percent;
  90
+}
5  xbmc/dialogs/GUIDialogBusy.h
@@ -32,10 +32,15 @@ class CGUIDialogBusy: public CGUIDialog
32 32
   virtual bool OnBack(int actionID);
33 33
   virtual void DoProcess(unsigned int currentTime, CDirtyRegionList &dirtyregions);
34 34
   virtual void Render();
  35
+  /*! \brief set the current progress of the busy operation
  36
+   \param progress a percentage of progress
  37
+   */
  38
+  void SetProgress(float progress);
35 39
 
36 40
   bool IsCanceled() { return m_bCanceled; }
37 41
 protected:
38 42
   virtual void Show_Internal(); // modeless'ish
39 43
   bool m_bCanceled;
40 44
   bool m_bLastVisible;
  45
+  float m_progress; ///< current progress
41 46
 };
6  xbmc/filesystem/Directory.cpp
@@ -162,9 +162,15 @@ bool CDirectory::GetDirectory(const CStdString& strPath, CFileItemList &items, c
162 162
             {
163 163
               CSingleLock lock(g_graphicsContext);
164 164
 
  165
+              // update progress
  166
+              float progress = pDirectory->GetProgress();
  167
+              if (progress > 0)
  168
+                dialog->SetProgress(progress);
  169
+
165 170
               if(dialog->IsCanceled())
166 171
               {
167 172
                 cancel = true;
  173
+                pDirectory->CancelDirectory();
168 174
                 break;
169 175
               }
170 176
 
11  xbmc/filesystem/IDirectory.h
@@ -70,6 +70,17 @@ class IDirectory
70 70
    */
71 71
   virtual bool GetDirectory(const CStdString& strPath, CFileItemList &items) = 0;
72 72
   /*!
  73
+   \brief Retrieve the progress of the current directory fetch (if possible).
  74
+   \return the progress as a float in the range 0..100.
  75
+   \sa GetDirectory, CancelDirectory
  76
+   */
  77
+  virtual float GetProgress() const { return 0.0f; };
  78
+  /*!
  79
+   \brief Cancel the current directory fetch (if possible).
  80
+   \sa GetDirectory
  81
+   */
  82
+  virtual void CancelDirectory() { };
  83
+  /*!
73 84
   \brief Create the directory
74 85
   \param strPath Directory to create.
75 86
   \return Returns \e true, if directory is created or if it already exists
64  xbmc/filesystem/PluginDirectory.cpp
@@ -464,6 +464,7 @@ bool CPluginDirectory::WaitOnScriptResult(const CStdString &scriptPath, const CS
464 464
 
465 465
   unsigned int startTime = XbmcThreads::SystemClockMillis();
466 466
   CGUIDialogProgress *progressBar = NULL;
  467
+  bool cancelled = false;
467 468
 
468 469
   CLog::Log(LOGDEBUG, "%s - waiting on the %s plugin...", __FUNCTION__, scriptName.c_str());
469 470
   while (true)
@@ -491,7 +492,7 @@ bool CPluginDirectory::WaitOnScriptResult(const CStdString &scriptPath, const CS
491 492
     }
492 493
 
493 494
     // check whether we should pop up the progress dialog
494  
-    if (!progressBar && XbmcThreads::SystemClockMillis() - startTime > timeBeforeProgressBar)
  495
+    if (!retrievingDir && !progressBar && XbmcThreads::SystemClockMillis() - startTime > timeBeforeProgressBar)
495 496
     { // loading takes more then 1.5 secs, show a progress dialog
496 497
       progressBar = (CGUIDialogProgress *)g_windowManager.GetWindow(WINDOW_DIALOG_PROGRESS);
497 498
 
@@ -515,46 +516,35 @@ bool CPluginDirectory::WaitOnScriptResult(const CStdString &scriptPath, const CS
515 516
 
516 517
     if (progressBar)
517 518
     { // update the progress bar and check for user cancel
518  
-      if (retrievingDir)
519  
-      {
520  
-        CStdString label;
521  
-        if (m_totalItems > 0)
522  
-        {
523  
-          label.Format(g_localizeStrings.Get(1042).c_str(), m_listItems->Size(), m_totalItems);
524  
-          progressBar->SetPercentage((int)((m_listItems->Size() * 100 ) / m_totalItems));
525  
-          progressBar->ShowProgressBar(true);
526  
-        }
527  
-        else
528  
-          label.Format(g_localizeStrings.Get(1041).c_str(), m_listItems->Size());
529  
-        progressBar->SetLine(2, label);
530  
-      }
531 519
       progressBar->Progress();
532 520
       if (progressBar->IsCanceled())
533 521
       { // user has cancelled our process - cancel our process
534  
-        if (!m_cancelled)
535  
-        {
536  
-          m_cancelled = true;
537  
-          startTime = XbmcThreads::SystemClockMillis();
538  
-        }
539  
-        if (m_cancelled && XbmcThreads::SystemClockMillis() - startTime > timeToKillScript)
540  
-        { // cancel our script
  522
+        m_cancelled = true;
  523
+      }
  524
+    }
  525
+    if (!cancelled && m_cancelled)
  526
+    {
  527
+      cancelled = true;
  528
+      startTime = XbmcThreads::SystemClockMillis();
  529
+    }
  530
+    if (cancelled && XbmcThreads::SystemClockMillis() - startTime > timeToKillScript)
  531
+    { // cancel our script
541 532
 #ifdef HAS_PYTHON
542  
-          int id = g_pythonParser.getScriptId(scriptPath.c_str());
543  
-          if (id != -1 && g_pythonParser.isRunning(id))
544  
-          {
545  
-            CLog::Log(LOGDEBUG, "%s- cancelling plugin %s", __FUNCTION__, scriptName.c_str());
546  
-            g_pythonParser.stopScript(id);
547  
-            break;
548  
-          }
549  
-#endif
550  
-        }
  533
+      int id = g_pythonParser.getScriptId(scriptPath.c_str());
  534
+      if (id != -1 && g_pythonParser.isRunning(id))
  535
+      {
  536
+        CLog::Log(LOGDEBUG, "%s- cancelling plugin %s", __FUNCTION__, scriptName.c_str());
  537
+        g_pythonParser.stopScript(id);
  538
+        break;
551 539
       }
  540
+#endif
552 541
     }
553 542
   }
  543
+
554 544
   if (progressBar)
555 545
     CApplicationMessenger::Get().Close(progressBar, false, false);
556 546
 
557  
-  return !m_cancelled && m_success;
  547
+  return !cancelled && m_success;
558 548
 }
559 549
 
560 550
 void CPluginDirectory::SetResolvedUrl(int handle, bool success, const CFileItem *resultItem)
@@ -626,3 +616,15 @@ void CPluginDirectory::SetProperty(int handle, const CStdString &strProperty, co
626 616
   CPluginDirectory *dir = globalHandles[handle];
627 617
   dir->m_listItems->SetProperty(strProperty, strValue);
628 618
 }
  619
+
  620
+void CPluginDirectory::CancelDirectory()
  621
+{
  622
+  m_cancelled = true;
  623
+}
  624
+
  625
+float CPluginDirectory::GetProgress() const
  626
+{
  627
+  if (m_totalItems > 0)
  628
+    return (m_listItems->Size() * 100.0f) / m_totalItems;
  629
+  return 0.0f;
  630
+}
2  xbmc/filesystem/PluginDirectory.h
@@ -47,6 +47,8 @@ class CPluginDirectory : public IDirectory
47 47
   virtual bool GetDirectory(const CStdString& strPath, CFileItemList& items);
48 48
   virtual bool IsAllowed(const CStdString &strFile) const { return true; };
49 49
   virtual bool Exists(const char* strPath) { return true; }
  50
+  virtual float GetProgress() const;
  51
+  virtual void CancelDirectory();
50 52
   static bool RunScriptWithParams(const CStdString& strPath);
51 53
   static bool GetPluginResult(const CStdString& strPath, CFileItem &resultItem);
52 54
 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.