[pacman-dev] output issues status
So, a while ago I reported some output issues here, making the distinction between two kind : http://www.archlinux.org/pipermail/pacman-dev/2007-September/009317.html The first kind was worked around by Dan by removing these pointless "done" messages : http://projects.archlinux.org/git/?p=pacman.git;a=commit;h=3017b71cb5cde3aef... For the second one, Dan and Aaron had an argument about how to solve it. Dan has a preference for delaying the output, and Aaron prefers padding with space (the way it was in 3.0). 22:56 phrakture >> i don't get why you dislike padding with spaces. lots of other apps do that 22:57 phrakture >> i personally think it's a far better idea than blocking message output 22:57 phrakture >> because, well, what if we have a front end that doesn't output progress 22:57 phrakture >> and thus has no progress callback 22:59 toofishes >> we aren't blocking message output *from the backend* 22:59 toofishes >> this is all in the frontend! 23:00 toofishes >> if you had an install where there were 20 permission errors, we are going to have 20 progress bar fragments right now. 23:00 phrakture >> right, and I'm saying we can solve that with maybe 3 lines of code 23:00 toofishes >> ok, patches welcome :) 23:00 phrakture >> but it's padding, and you dislike that 23:01 toofishes >> well i haven't seen your implementation. maybe i won't dislike it. 23:01 toofishes >> i just feel like this isn't the right way or something. Since Aaron promised restoring the padding way in 3 lines, I couldn't really compete with that ;), and besides 3.0 already has a sample implementation. So I tried exploring the delayed output way instead, and hacked something together in ~50 lines. It's very ugly and hackish, but it's just meant as a proof of concept, because I'm not even sure the idea is right.
From 9de58523ee64c4c2d86148fad30be1c6cf543d22 Mon Sep 17 00:00:00 2001 From: Chantry Xavier
Date: Mon, 3 Dec 2007 22:57:54 +0100 Subject: [PATCH] Delay output during progress bar.
This is just a proof of concept patch to fix the output issue related to the
progress bar by delaying the output.
Signed-off-by: Chantry Xavier
On Dec 3, 2007 4:20 PM, Xavier
Since Aaron promised restoring the padding way in 3 lines, I couldn't really compete with that ;), and besides 3.0 already has a sample implementation. So I tried exploring the delayed output way instead, and hacked something together in ~50 lines. It's very ugly and hackish, but it's just meant as a proof of concept, because I'm not even sure the idea is right.
Hah, yeah I actually assumed some sort of vasprintf-esque function, so I would have counted that too. Still, I think that for the time being a "hackish" solution is best (even though this one is pretty good), because we just need to do some cleanup to get 3.1 out the door. I like it.
On Mon, Dec 03, 2007 at 04:29:58PM -0600, Aaron Griffin wrote:
On Dec 3, 2007 4:20 PM, Xavier
wrote: Since Aaron promised restoring the padding way in 3 lines, I couldn't really compete with that ;), and besides 3.0 already has a sample implementation. So I tried exploring the delayed output way instead, and hacked something together in ~50 lines. It's very ugly and hackish, but it's just meant as a proof of concept, because I'm not even sure the idea is right.
Hah, yeah I actually assumed some sort of vasprintf-esque function, so I would have counted that too.
Still, I think that for the time being a "hackish" solution is best (even though this one is pretty good), because we just need to do some cleanup to get 3.1 out the door.
I like it.
Oh, ok then. Let's see if this "hackish" solution is also good enough for Dan, and then the eventual cleanup / refactoring / renaming that needs to be done, as well as commenting this hack in the code. I originally tried to base the detection of the progress bar in cb_trans_evt, based on the events sent before and after the progress bar. But there were the different upgrade / add / remove cases to consider, and it looked a bit messy. So I tried to move it at the end of cb_trans_progress, based on the "int percent" argument, and it turned out to be easier, and better factored for the different cases. Anyway, I wasn't familiar with this part of the code at all, so this hack should be carefully reviewed before being considered to be merged :)
On Dec 3, 2007 4:50 PM, Xavier
On Mon, Dec 03, 2007 at 04:29:58PM -0600, Aaron Griffin wrote:
On Dec 3, 2007 4:20 PM, Xavier
wrote: Since Aaron promised restoring the padding way in 3 lines, I couldn't really compete with that ;), and besides 3.0 already has a sample implementation. So I tried exploring the delayed output way instead, and hacked something together in ~50 lines. It's very ugly and hackish, but it's just meant as a proof of concept, because I'm not even sure the idea is right.
Hah, yeah I actually assumed some sort of vasprintf-esque function, so I would have counted that too.
Still, I think that for the time being a "hackish" solution is best (even though this one is pretty good), because we just need to do some cleanup to get 3.1 out the door.
I like it.
Oh, ok then. Let's see if this "hackish" solution is also good enough for Dan, and then the eventual cleanup / refactoring / renaming that needs to be done, as well as commenting this hack in the code.
For now, this looks quite good enough. Great work. You did this as cleanly as one would possibly be able to- the only other concern would be with a interrupt, which I haven't tested.
I originally tried to base the detection of the progress bar in cb_trans_evt, based on the events sent before and after the progress bar. But there were the different upgrade / add / remove cases to consider, and it looked a bit messy. So I tried to move it at the end of cb_trans_progress, based on the "int percent" argument, and it turned out to be easier, and better factored for the different cases.
Anyway, I wasn't familiar with this part of the code at all, so this hack should be carefully reviewed before being considered to be merged :)
I'm going to let it bake on my working branch tonight (I made a few small changes but nothing huge). If everyone could give it a test that would be great. -Dan
On Mon, Dec 03, 2007 at 06:42:11PM -0600, Dan McGee wrote:
For now, this looks quite good enough. Great work. You did this as cleanly as one would possibly be able to- the only other concern would be with a interrupt, which I haven't tested.
I'm going to let it bake on my working branch tonight (I made a few small changes but nothing huge). If everyone could give it a test that would be great.
Nothing wrong with your small changes, but it reminded me of something I didn't mention: the delayed output is only done when --debug is NOT set, because when --debug is used, the progress bar is disabled with noprogressbar = 1. So in pm_vasprintf, the logmask will never overlap with LOG_DEBUG or LOG_FUNCTION. However, that only concerns this particular usage of pm_vasprintf, so you were right to add LOG_DEBUG and LOG_FUNCTION handling to pm_vasprintf. It just isn't used currently.
On Dec 4, 2007 12:11 PM, Xavier
On Mon, Dec 03, 2007 at 06:42:11PM -0600, Dan McGee wrote:
For now, this looks quite good enough. Great work. You did this as cleanly as one would possibly be able to- the only other concern would be with a interrupt, which I haven't tested.
I'm going to let it bake on my working branch tonight (I made a few small changes but nothing huge). If everyone could give it a test that would be great.
Nothing wrong with your small changes, but it reminded me of something I didn't mention: the delayed output is only done when --debug is NOT set, because when --debug is used, the progress bar is disabled with noprogressbar = 1. So in pm_vasprintf, the logmask will never overlap with LOG_DEBUG or LOG_FUNCTION.
However, that only concerns this particular usage of pm_vasprintf, so you were right to add LOG_DEBUG and LOG_FUNCTION handling to pm_vasprintf. It just isn't used currently.
Yeah, I realized this, but one of two things should be done: 1. Copy the existing switch statement with all the options (as I ended up doing). 2. Make an explicit comment in the code why certain statements were omitted. -Dan
On Tue, Dec 04, 2007 at 01:04:28PM -0600, Dan McGee wrote:
Yeah, I realized this, but one of two things should be done: 1. Copy the existing switch statement with all the options (as I ended up doing). 2. Make an explicit comment in the code why certain statements were omitted.
Ok, fair enough. As I already said, I'm fine with 1. I thought it might still be worth adding a little comment where pm_vasprintf is called, in cb_log, but I actually wouldn't know what to write exactly, and I'm not sure it would be really useful, so forget it.
participants (3)
-
Aaron Griffin
-
Dan McGee
-
Xavier