-
Notifications
You must be signed in to change notification settings - Fork 0
Centralized progress dialog #35
base: development
Are you sure you want to change the base?
Conversation
|
||
public: | ||
Steps mSteps; | ||
Status mStatus = Running; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't do this, unless it's a static initialiser. Confusion in constructors as what happens when is too error-prone otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, okay ... You mean like error-prone concerning code readability or technically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
@scunz Btw.: Could you please fix the following compiler error in [ 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! |
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 This code has to be part of the I'm just about to finish a demo for a |
Let me see, if I forgot to commit/push something. However, the right fix is to replace |
I've done that change and pushed it. |
42cdc5b
to
26abca7
Compare
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. |
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:
@BugSniffer FYI 👀 |
{ | ||
Activities::Iterator it = mActivities.begin(); | ||
while ( it != mActivities.end() ) { | ||
Private::ProgressWdgt* a = *it; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a nullptr
check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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 |
@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 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 .... 😄 |
Inside
It has to be ported to |
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". |
I totally agree to that!!! A few things that I'd like to be changed though:
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 |
|
||
a->mActive = false; | ||
a->txtStatusInfo->setText(tr("Finished!")); | ||
a->resultChanged(Private::ProgressWdgt::Result::Ok); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I actually want to use enum class
from now on, and change the traditional way - thanks! :)
And that's exactly why I didn't implement it yet - I'd have to do it twice :). |
I'll post a screenshot ... |
This is how the "inlined" text looks like: Looks pretty good to me.
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 *** Error in `MacGitver': double free or corruption (fasttop): 0x0000000001dd1650 *** 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 So, yes. I'm already on it :) Back to the topic:
Exactly! Couldn't agree more. This is what we need.
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... |
Actually: Your perception of which thread sends/receives the |
e8aac25
to
b07bf35
Compare
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
…ag better reflects what I mean here
…viy's result and minor cleanup
When a step progress reaches 100%, the progress bar changes to a green background.
b07bf35
to
c59f39c
Compare
c59f39c
to
2676269
Compare
That looks like a good starting point |
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 The 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.
Provide a central place to visualize progress of running operations.
ProgressDlg
from "Repository" module tolibMacGitverCore
libActivitiesGui
.libActivitiesGui
.libMacGitverCore
.Activity::Manage??
.