Centralized progress dialog

[antis81]

Provide a central place to visualize progress of running operations.

• [x] Part 1
  • [x] Copy existing ProgressDlg from "Repository" module to libMacGitverCore
  • Side note: The dialog was used in the "CloneDialog".
  • [x] Make it generic :arrow_right: redefine slots to handle reported progress)
  • [x] Add concept to add an activity, consisting of multiple steps.
  • [x] Make it look good. :star2:
• [ ] Part 2
  • [x] Create a libActivitiesGui.
  • [x] Copy ProgressDlg to libActivitiesGui.
  • [ ] Delete ProgressDlg in libMacGitverCore.
  • [ ] Make it a single instance, managed by Activity::Manage??.
  • [ ] Reimplement & rewire to Activity framework.
  • [ ] Rethink the logging: Each activity (service?) has it's own log.

[antis81]

@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]

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]

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]

I've done that change and pushed it.

[antis81]

... 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]

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 :zap: - something went wrong:

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

1. :beetle: :zap: 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).
2. The open points on the top :smile:

@BugSniffer FYI :eyes:

[antis81]

@scunz Thanks for the hints concerning the crash. Unfortunately (actually fortunately :smile:) 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 :smile:. I'll recheck to verify, but likely there's no problem at all.

[antis81]

@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 .... :smile:

[scunz]

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]

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]

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...

[antis81]

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]

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]

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]

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]

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.

[scunz]

That looks like a good starting point

[antis81]

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 :star2: :smile:.