Centralized progress dialog #35

antis81 · 2015-04-25T15:33:16Z

Provide a central place to visualize progress of running operations.

scunz · 2015-04-25T16:58:00Z

Libs/libMacGitverCore/Widgets/ProgressDlg.cpp

+
+    public:
+        Steps   mSteps;
+        Status  mStatus     = Running;


Please don't do this, unless it's a static initialiser. Confusion in constructors as what happens when is too error-prone otherwise.

Hm, okay ... You mean like error-prone concerning code readability or technically?

Both, actually. If I found a constructor only initialising half of the members, I'd probably consider this an error and fix it - though there might not be a problem at all, considering that some member could be initialised in the class definition.

Technically spoken: At which point can you assume which variable to be initialised? We know the order changed from declaration-order to access-then-declaration-order from C++03 to C++11. Trouble starts when you have to use this inside a constructor's base-class or member-initialiser-list...

antis81 · 2015-04-25T17:16:39Z

@scunz Btw.: Could you please fix the following compiler error in development branch (just a little one) and make it compile again? I can rebase this PR afterwards.

[ 13%] Building CXX object Libs/libMacGitverCore/CMakeFiles/MacGitverCore.dir/RepoMan/AutoRefresher.cpp.o                       
/home/nils/Projects/MacGitver/Libs/libMacGitverCore/RepoMan/AutoRefresher.cpp:196:9: error: no matching function for call to    
      'log'
        MacGitver::log().addMessage(trUtf8("Refreshing git repositories..."));
        ^~~~~~~~~~~~~~
/home/nils/Projects/MacGitver/Libs/libMacGitverCore/App/MacGitver.hpp:66:17: note: candidate function not viable: requires 2    
      arguments, but 0 were provided
    static void log( Log::Type type, const QString& logMessage );

Thanks!

scunz · 2015-04-25T17:17:51Z

So, I've been reviewing this code now 3 or 4 times - and this time I've left some traces of it :)

However, when I first read it, I was worried about one thing: Who's going to synchronise this code - I've thought for quite a time that it's possible to do that in the ActivityManager, but reading it over and over again and thinking about how I'm going to do that ActivityManager thing, I've come to the conclusion that it's not possible.

This code has to be part of the ActivityManager - otherwise, we cannot synchronise it correctly. And the ActivityManager has to assume a very proactive role in this process (I actually think that you've been after that anyway).

I'm just about to finish a demo for a MagicDialog that I'd really like to be used as a base for what you have now as ExpandDialog. After I've finished that demo, I'll focus on the ActivityManager. I think that I should in general focus on the RepoMan stuff and leave the UI stuff with you for now - OTOH I'd really like to see my magic dialog as a base, so I'd ask you if you could take that code then and properly integrate it into core. Would that be okay with you?

scunz · 2015-04-25T17:18:42Z

Could you please fix the following compiler error

Let me see, if I forgot to commit/push something. However, the right fix is to replace MacGitver::log() with Log::Manager().

scunz · 2015-04-25T17:24:09Z

I've done that change and pushed it.

antis81 · 2015-04-25T20:34:22Z

... OTOH I'd really like to see my magic dialog as a base, so I'd ask you if you could take that code then and properly integrate it into core. Would that be okay with you?

Yeah totally fine. The current ExpandableDlg has a little problem anyways. Unlike the first implementation, the options widget is always instantiated. This currently leads to the fact, that all options are actually set.

antis81 · 2015-05-01T13:24:25Z

From a visual perspective, the dialog got a really nice yet simple look; some impressions taken from a test clone of Qt-Creator:

By default, an activity will appear collapsed:

Expanded - all activities finished;

Oops ⚡ - something went wrong:

The code design could be better. Mainly two points are holding me back to merge this:

🪲 ⚡ The dialog causes casual crashes and I'm not quite sure, where to fix that. @scunz Some review could help (see the timer method ProgressDlg::updateActivities).
The open points on the top 😄

@BugSniffer FYI 👀

scunz · 2015-05-01T13:48:50Z

Libs/libMacGitverCore/Widgets/ProgressDlg.cpp

+{
+    Activities::Iterator it = mActivities.begin();
+    while ( it != mActivities.end() ) {
+        Private::ProgressWdgt* a = *it;


Missing a nullptr check here.

No - code flow is sequential. That means any instance of ProgressWdgt is destroyed only in the destructor of ProgressDlg. A ProgressWdgt must not be destroyed before the assigned running activity has stopped (ok or with error).

IOW: Each ProgressWdgt is valid until the ProgressDlg is closed/destroyed.

I'll put an assert as a reminder here anyways.

I think that that assumption is bad. The ProgressDlg ought to live forever. Hence, when the activity is finished, the progress widgets ought to be removed. However, we should not do that immediately - So maybe this needs a "dismiss" mechanism; and when doing that we might as well introduce a "cancel" mechanism (The UI ought to be the same, but the implementation will be quite different / difficult).

Anyway, if your assumption that a widget is not deleted before the dialog is true, then there's no need to store them inside QPointer<>'s.

antis81 · 2015-05-02T09:49:48Z

@scunz Thanks for the hints concerning the crash. Unfortunately (actually fortunately 😄) both are not relevant to the "problem". I had some uncommitted changes to GitWrap, playing with the destructors in Git::CloneOperation and it's base classes. After I reverted those changes, I cannot reproduce the crashes anymore - which is good actually 😄. I'll recheck to verify, but likely there's no problem at all.

antis81 · 2015-05-05T07:39:28Z

@scunz Our progress dialog is now available from a central place, but still instantiated per activity. The concept requires a single instance, where activities are added (registered) and then run in parallel. Where would you like that to be added?

When this is done, the dialog is fairly complete and usable. I plan to merge at this point. It is however not yet possible to register subactivities (we could use this to realize git clone --recursive).

This is only a first small step in the right direction. When it comes to visual appearance, I take Firefox and Qt-Creator as an inspirational source. There's only an icon, showing a summed progress bar of all running activities. Clicking on it (keyboard shortcut) reveals the detailed dialog. However. This is for the future .... 😄

scunz · 2015-05-06T19:43:55Z

Where would you like that to be added?

Inside libActivities :)

When this is done, the dialog is fairly complete and usable. I plan to merge at this point.

It has to be ported to libActivities. Sorry that I've still not finished it, but should be soonish now.

scunz · 2015-05-06T21:06:10Z

Btw.: The middle step of the clone operation is not about adding objects to the git index. It's about creating an inventory for the downloaded pack file. That's why the total number is equal to the amount of downloaded objects. The total number would be equal to the number of files + directories, if this was about building the actual "git index".
The "git index" is probably created directly before the checkout (iirc libgit2 can actually only checkout indicies not trees). But I'm not sure about that.

scunz · 2015-05-06T21:11:42Z

This is only a first small step in the right direction.

I totally agree to that!!!

A few things that I'd like to be changed though:

Can you reduce the size of the widget and remove the "finished" text once the activity is done?
Actually: Can't we drop that line completely?

Could we try a little experiment: When this "Downloaded 500 of 1000 objects (1234mb)" text is set, could we try to show that text inside the progress bar instead of the "50%"? Would be awesome if that's possible!

I reckon that you're right now lacking a mechanism to show "details" as they come from the side-band channel in the git protocol. I can't actually see any other use of the "details" pane. But I'll implement a way to do that in libActivities respecting all the peculiarities of the side band transfer...

scunz · 2015-05-06T21:21:58Z

Libs/libMacGitverCore/Widgets/ProgressDlg.cpp

+
+    a->mActive = false;
+    a->txtStatusInfo->setText(tr("Finished!"));
+    a->resultChanged(Private::ProgressWdgt::Result::Ok);


Just a side node:
enum doesn't introduce a name scope (C++11's enum class does). Thus, when you refer to Result::Ok some (i.e. Microsoft's) compilers complain. OTOH if you're using enum class, then the scope name (the enum's name) is mandatory.

namespace X { enum Y { Z }; } // --> X::Z namespace X { enum class Y { Z }; } // --> X::Y::Z // And more interesting: enum class Foo { None, Fooish }; enum class Bar { None, Barish }; Foo f = Foo::None; Bar b = Bar::None; // Contrary to traditional way: enum Foo { FooNone, FooFooish }; enum Bar { BarNone, BarBarish };

Yes, I actually want to use enum class from now on, and change the traditional way - thanks! :)

antis81 · 2015-05-07T06:17:25Z

I reckon that you're right now lacking a mechanism to show "details" as they come from the side-band channel in the git protocol. I can't actually see any other use of the "details" pane. But I'll implement a way to do that in libActivities respecting all the peculiarities of the side band transfer...

And that's exactly why I didn't implement it yet - I'd have to do it twice :).

antis81 · 2015-05-07T06:24:42Z

Could we try a little experiment: When this "Downloaded 500 of 1000 objects (1234mb)" text is set, could we try to show that text inside the progress bar instead of the "50%"? Would be awesome if that's possible!

I'll post a screenshot ...

antis81 · 2015-05-07T12:48:32Z

This is how the "inlined" text looks like:

Looks pretty good to me.

If you're ok with it, I'll also completely remove the "Running ...", "Finished!" and "Failed!" texts.
- (Note the missing "Not started ..." text in the "Checkout" step.)
Need a replacement word (e.g. "Packed") to correct the text "Indexed 123/234 objects".

Off-Topic, but important: The further above mentioned crash fortunately reappeared ... it is a good old threading problem. The problem code lives in the File CloneRepositoryDlg.cpp in the related branch ngf/reorganize-clone-dlg. The rootCloneFinished() slot is called by the worker thread (see Git::BaseOperation::workerFinished()). The line delete operation; causes the trouble now and can crash with a "double free" error:

*** Error in `MacGitver': double free or corruption (fasttop): 0x0000000001dd1650 ***

Could you please revisit this and help to get it right?

scunz · 2015-05-08T19:50:25Z

Could you please revisit this and help to get it right?

The fix is simple:

cd $GITWRAP
git rm -rf libGitWrap/Operations
git push --force origin development

Seriously, the gitWrap operations are fucked up beyond repair - and I think that this is no news, actually. That's why we're doing all this repoman stuff :p

So, yes. I'm already on it :)

Back to the topic:

Looks pretty good to me.

Exactly! Couldn't agree more. This is what we need.

Need a replacement word (e.g. "Packed") to correct the text "Indexed 123/234 objects".

No, the wording is good now. These objects are indexed - they are just not added to the git index. "Katalogisiert" would be a better word in German, but it sounds strange in English...

scunz · 2015-05-08T20:28:49Z

Actually: Your perception of which thread sends/receives the finished() signal seems wrong. If I read the code correctly, this ought to happen in the thread which created the operation. However, I can't see why it's going wrong at a short sight of the code.

These tests actually didn't test very much, are in the wrong library and no longer compatible with how the RepoMan will work. We need to investigate further in how to do actually useful testing of the RepoMan.

Activities become invalid, when they where deleted (pointer == nullptr).

… the progress bar

…on painting

…ag better reflects what I mean here

…viy's result and minor cleanup

…st* be valid

When a step progress reaches 100%, the progress bar changes to a green background.

…essDlg files

scunz · 2015-05-21T15:56:15Z

That looks like a good starting point

antis81 · 2015-05-23T11:58:43Z

I just started reimplementing the dialog (and thus, this PR), switching to an item based approach - no big surprise. I already stated out, that this PR was mainly a visual preview of what the result can look like and to "get a feeling" for what we need. Nothing more, nothing less.

I'm changing the current implementation to one, based on QStandardItemModel (maybe QAbstractItemModel). Actually we have a simple tree of items of the same type (ProgressItem). The code in 70cc3c0 reuses your concept from Module/References/Branches/RefsItem.hpp. The base classes are a good candidate for libMacGitverCoreGui - creates a common TreeItem and TreeItemObject. In the context of this PR, the data object will refer to either a Service, Activity or Step class. As I can see now, we don't require much of a differentiation between those types. They all should have the same data fields available (displayName, curProgress, info, ...). Anyways,

The ProgressWdgt will be trashed and I'm going to "custom draw" the item's rect completely by the view/delegate. This is laying out a path, allowing us to go far beyond what we have now - future music.

For now, I'll keep the dialog and finish the things under the hood first. Actually I imagine to invent a new mode (some "activities" mode with an animated icon in the left side bar). This would add some real visual pepper while creating a clean user experience the same time 🌟 😄.

The delegate is just a template and not done right yet.

antis81 changed the title ~~Ngf/central progress~~ Centralized progress dialog Apr 25, 2015

This was referenced Apr 25, 2015

Reorganize clone dialog #32

Closed

Reorganize clone dialog #36

Open

scunz reviewed Apr 25, 2015
View reviewed changes

antis81 force-pushed the ngf/central-progress branch from 42cdc5b to 26abca7 Compare April 25, 2015 18:10

scunz reviewed May 1, 2015
View reviewed changes

scunz reviewed May 6, 2015
View reviewed changes

antis81 force-pushed the ngf/central-progress branch 2 times, most recently from e8aac25 to b07bf35 Compare May 9, 2015 14:33

scunz added 3 commits May 11, 2015 01:04

Remove useless RepoMan tests

3391236

These tests actually didn't test very much, are in the wrong library and no longer compatible with how the RepoMan will work. We need to investigate further in how to do actually useful testing of the RepoMan.

Remove signals from RM::Repo

4cb1466

Remove 'open' logic'

66ff7eb

antis81 added 20 commits May 17, 2015 13:37

ProgressDlg: removed customized palette for log view

5d3995f

ProgressDlg: minor cleanup

2152781

ProgressDlg: add methods isDone() and setCanClose()

5a1bc44

ProgressDlg: move asserts

b026b9e

ProgressDlg: remove invalid activities from progress updating

0aed968

Activities become invalid, when they where deleted (pointer == nullptr).

add ProgressDlg::setError()

d6a6c8f

ProgressDlg: prevent accept() or reject() before activities are finished

294c2f2

PrgressDlg: frequently update close buttons

2627e42

ProgressDlg: set step percentage to 100% when finished

dc05e54

ProgressDlg: mark finished and error state of an activity by coloring…

eb6eac1

… the progress bar

ProgressDlg: fixed erase activities

825d4e8

ProgressDlg: make internal progress widgets use the cells background …

c865406

…on painting

ProgressDlg: a progress widget has no "running" state; an "active" fl…

444f107

…ag better reflects what I mean here

ProgressDlg: refine the appearance of progress bars by using the acti…

7ab5ac7

…viy's result and minor cleanup

ProgressDlg: add an assert, because currently the activity widget *mu…

df89eb2

…st* be valid

ProgressDlg: inline the info text in the progress bar

7e13dfb

ProgressDlg: use C++11 enum class

26cc7fa

ProgressDlg: remove "running" and "finished" text

a996865

ProgressDlg: add a more descriptive "failed" text

6aea9a2

ProgressDlg: only the activity has a finished slot

75433fe

When a step progress reaches 100%, the progress bar changes to a green background.

antis81 force-pushed the ngf/central-progress branch from b07bf35 to c59f39c Compare May 17, 2015 11:37

antis81 force-pushed the development branch from b82ee30 to 84989f0 Compare May 18, 2015 08:10

Activities-Gui: add libActivitiesGui static library with copied Progr…

2676269

…essDlg files

antis81 force-pushed the ngf/central-progress branch from c59f39c to 2676269 Compare May 21, 2015 07:37

Activities-Gui: draft implementation of ProgressItem classes

70cc3c0

antis81 added 3 commits June 1, 2015 22:43

ProgressDlg: added data fields to the ProgressItem

9e5fcaa

ProgressDlg: add parent for each ProgressItem

6790840

ProgressDlg: add item delegate to draw the items

243398d

The delegate is just a template and not done right yet.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralized progress dialog #35

Centralized progress dialog #35

antis81 commented Apr 25, 2015

scunz Apr 25, 2015

antis81 Apr 25, 2015

scunz Apr 25, 2015

antis81 commented Apr 25, 2015

scunz commented Apr 25, 2015

scunz commented Apr 25, 2015

scunz commented Apr 25, 2015

antis81 commented Apr 25, 2015

antis81 commented May 1, 2015

scunz May 1, 2015

antis81 May 2, 2015

scunz May 6, 2015

antis81 commented May 2, 2015

antis81 commented May 5, 2015

scunz commented May 6, 2015

scunz commented May 6, 2015

scunz commented May 6, 2015

scunz May 6, 2015

antis81 May 7, 2015

antis81 commented May 7, 2015

antis81 commented May 7, 2015

antis81 commented May 7, 2015

scunz commented May 8, 2015

scunz commented May 8, 2015

scunz commented May 21, 2015

antis81 commented May 23, 2015

Centralized progress dialog #35

Are you sure you want to change the base?

Centralized progress dialog #35

Conversation

antis81 commented Apr 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antis81 commented Apr 25, 2015

scunz commented Apr 25, 2015

scunz commented Apr 25, 2015

scunz commented Apr 25, 2015

antis81 commented Apr 25, 2015

antis81 commented May 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antis81 commented May 2, 2015

antis81 commented May 5, 2015

scunz commented May 6, 2015

scunz commented May 6, 2015

scunz commented May 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antis81 commented May 7, 2015

antis81 commented May 7, 2015

antis81 commented May 7, 2015

scunz commented May 8, 2015

scunz commented May 8, 2015

scunz commented May 21, 2015

antis81 commented May 23, 2015