After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 576934 - Make width of file and folder comparison panes adjustable
Make width of file and folder comparison panes adjustable
Status: RESOLVED FIXED
Product: meld
Classification: Other
Component: filediff
git master
Other Linux
: Normal enhancement
: ---
Assigned To: Stephen Kennedy
Stephen Kennedy
: 700290 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2009-03-27 07:40 UTC by Mark Doliner
Modified: 2014-06-09 21:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
The diff (63.68 KB, patch)
2009-03-27 07:42 UTC, Mark Doliner
needs-work Details | Review
The complete file glade2/filediff.glade (34.32 KB, text/plain)
2009-03-27 07:43 UTC, Mark Doliner
  Details
Screenshot of the change in action (114.69 KB, image/png)
2009-03-27 07:45 UTC, Mark Doliner
  Details
Kompare screenshot (65.62 KB, image/png)
2009-03-27 09:00 UTC, Vincent Legoll
  Details
Reimplementation of previous patch for current Meld (52.90 KB, patch)
2013-07-27 21:33 UTC, Kai Willadsen
needs-work Details | Review
Experiment to see the look and feel of a linkmap based from a GtkPaned. (49.51 KB, patch)
2014-03-18 13:47 UTC, Marco Brito
none Details | Review
Experiment with custom size allocation, GtkGrid based. (10.25 KB, patch)
2014-04-08 14:47 UTC, Marco Brito
none Details | Review
DiffGrid, experiment with custom size allocation. (14.71 KB, patch)
2014-04-15 15:04 UTC, Marco Brito
none Details | Review
DiffGrid, version 3. (15.66 KB, patch)
2014-05-01 14:15 UTC, Marco Brito
none Details | Review
DiffGrid, version 4. (15.76 KB, patch)
2014-05-20 12:29 UTC, Marco Brito
accepted-commit_after_freeze Details | Review
DiffGrid, version 5. (16.08 KB, patch)
2014-05-27 16:42 UTC, Marco Brito
none Details | Review
DiffGrid, version 6. (16.00 KB, patch)
2014-05-31 12:41 UTC, Marco Brito
none Details | Review
DiffGrid, version 7. (16.96 KB, patch)
2014-06-04 14:38 UTC, Marco Brito
none Details | Review

Description Mark Doliner 2009-03-27 07:40:47 UTC
The attached patch changes the filediff glade file.  It shuffles around the main table in the main container to put the scrolled windows inside two horizontal paned containers.  This has the benefit of adding the little grabby thing between panes that lets you adjust the size of each file viewer.  See the attached screenshot for an example of what I'm talking about.

It's useful when you have a lot of changes on one side of the diff and you want to be able to view more of that window.

This is really my first time using glade (but I've been using gtk2 for years), so I may have done something a little funky.  The filediff.glade file is like, TOTALLY different, and so I've attached the full version of that file in case it's easier to use.  I'm using glade 2.12.2 which ships with Ubuntu.  Maybe this is a different version than what you guys are using?  If it would help if I used a different version of glade just say the word.
Comment 1 Mark Doliner 2009-03-27 07:42:30 UTC
Created attachment 131485 [details] [review]
The diff
Comment 2 Mark Doliner 2009-03-27 07:43:20 UTC
Created attachment 131486 [details]
The complete file glade2/filediff.glade
Comment 3 Mark Doliner 2009-03-27 07:45:52 UTC
Created attachment 131487 [details]
Screenshot of the change in action
Comment 4 Vincent Legoll 2009-03-27 08:59:51 UTC
Ihave also been experimenting with that feature, but failed. I was trying to get the diffmap between the two textviews to act like a slider when grabbed with the mouse, it was somewhat working but the rate of size change was not the same as the mouse movement...

I absolutely love the idea to be able to change textviews width, like for example in araxis diffnmerge or in kompare. But I can't say I like the picture you're showing us, sorry. I would prefer something like the kompare screenshot.

RE glade, I think we all suffer from the same symptoms as you, mainly we have to open the file, save an unmodified version first, to have a comparison base, then hack on the widgets and diff with the base to manually report the changes in the source controlled file... That's really painful.
Comment 5 Vincent Legoll 2009-03-27 09:00:45 UTC
Created attachment 131488 [details]
Kompare screenshot
Comment 6 Mark Doliner 2009-03-30 19:47:28 UTC
Hi Vincent.  Out of curiosity, if the diffmap between the two textviews acts like a slider, is there any visual indication to the user that this area is clickable/draggable?
Comment 7 Vincent Legoll 2009-03-30 20:40:14 UTC
In my quick'n'dirty experimentation I only
hooked drag event to resize the textviews.

When you'll have that working well, you
can worry about a way to tell the user
about the resizability of the thing...

I stopped earlier than that, but the item
is still in my todo list...
Comment 8 Kai Willadsen 2009-03-31 04:55:55 UTC
Just a quick not on the Glade problem: I've found that the current glade series (3.5.x) does a massively better job in terms of keeping changes to a minimum. There will still be more than a few changes, some from manual editing etc., but it shouldn't rewrite the entire file like the 3.2/3.4 series tended to.
Comment 9 Stephen Kennedy 2009-04-03 07:04:25 UTC
Araxis does this rather well with the < > grabber at the top of the center column. I'd like this style if possible. http://pcwin.com/media/images/screen/64797-araxis_merge.gif

Vincent, I ran into a similar issue when trying this. I wonder if it's even possible without reimplementing the container interface?

Kai, glad to hear the new glade is better. I had started on a script to sort the glade xml tree for better & painless diffs, but I'll just wait for the next version.
Comment 10 Kai Willadsen 2013-05-17 21:45:44 UTC
*** Bug 700290 has been marked as a duplicate of this bug. ***
Comment 11 Kai Willadsen 2013-05-17 21:47:04 UTC
It's worth noting that with additional columns now in folder comparisons, this would probably make sense there too.
Comment 12 Adam Dingle 2013-07-26 15:42:15 UTC
I use Meld every day and this is my top Meld feature request at this time.  Kai, do you anticipate implementing this any time soon?
Comment 13 Kai Willadsen 2013-07-26 21:44:41 UTC
I'm taking a look. It might not be overly difficult to get something similar to the original patch going again, but I'd be very surprised if that didn't break other size allocations, etc.
Comment 14 Adam Dingle 2013-07-27 14:28:14 UTC
Great to hear you're looking at this - thanks!
Comment 15 Kai Willadsen 2013-07-27 21:33:57 UTC
Created attachment 250278 [details] [review]
Reimplementation of previous patch for current Meld

This is basically just a forward-port of the previous patch to current Meld. I had to lose some things while doing this (findbar alignment and the scroll lock button), but that's fine.


There are two show-stopper problems with doing this.

Firstly, it screws up GTK's size allocations. When doing this, you never actually get horizontal space split 50/50 by default, and some actions that shouldn't change allocations, do. I'm not sure what's causing this, since requisitions for both panes should be identical. This is a problem I remember having last time I looked at this.

Secondly, there is no custom drawing on the new HPaned widgets. This looks really bad and is in itself enough to stop me from doing this. We *could* get another custom widget (just a HPaned subclass I guess) to do this painting. That's kind of horrible, and will slow Meld down (another thing iterating over changes) but I don't have a better solution. It's also not a trivial quantity of work.

The better option would be to make LinkMap into a HPaned subclass, but having spent a couple of hours poking that I think it's a much bigger task than a new custom widget.


Short version is that I'm not willing to spend a lot of time on this immediately. Anyone who wants the adjustable panes and is willing to live with some broken behaviour can apply this patch. While I'd like this to happen, unless someone else picks up the ball it will take a while.
Comment 16 Adam Dingle 2013-07-30 01:15:45 UTC
Kai, you evidently put in a noble effort to try to fix this and I really appreciate that.  It sounds as if this isn't as easy as I had hoped.  Still, it's nice to have the patch you attached as a fallback option.  Sadly I have no time to look at this myself right now, but perhaps someone else out there who cares about this will pick this up.
Comment 17 Adam Dingle 2014-01-11 11:15:13 UTC
There's a bounty on this feature at Bountysource:

https://www.bountysource.com/issues/1072093-make-width-of-file-and-folder-comparison-panes-adjustable

On that page someone named "3adsada" said they had begun work on this (as of Jan 1).  3adsada, are you still working on this?  Do you have any progress to report?
Comment 18 Marco Brito 2014-03-10 11:02:26 UTC
Hi, i am lured from Bountysource.

At first sight this is looking more a design problem than tecnical.
For start, as some time already passed, i would like to ask to meld developers to give a updated vision for this feature.


I looked at it, and the current idea is to do a custom container to replace GtkGrid. This will not handle drawing, but only position the textview, linkmap, diffmap.

How should the resize be handled? Something like GtkPaned, a handle full height has the inconvenience of been in the way as linkmap requires to have width.
What about similar to GtkTreeView column resize, only in column headers the resize is allow, in our case the header is that menu over the textviews.
Comment 19 Marco Brito 2014-03-18 11:55:51 UTC
Hi again.
It would be good to create some discussion of how this feature should be implemented, in particular the design.
Comment 20 Marco Brito 2014-03-18 13:47:23 UTC
Created attachment 272271 [details] [review]
Experiment to see the look and feel of a linkmap based from a GtkPaned.

This patch is a experiment to see how is the look and feel of linkmap inside a GtkPaned. Is only to help find what can be done.

Please ignore dirdiff and the diffmap misalignments, only the minimum was done.
Comment 21 Kai Willadsen 2014-03-20 20:22:58 UTC
(In reply to comment #19)
> Hi again.
> It would be good to create some discussion of how this feature should be
> implemented, in particular the design.

Right. This feature has never really had a design. The prototype patch looks to me to be in the right direction, though as you say it's a minimal approach, and other options might be better.

My personal requirements (which may not be the same as the person who posted the bounty) are:

 * The UI should behave *exactly* as it does now, until the user tries to manipulate pane width. This includes things like getting default sizing right, etc. (which the current prototype doesn't)

 * Visual impact on the current UI should be minimal. We'll use the same scrollbar placement, and linkmaps should still have a seamless join with textviews.

 * Three pane mode has to work. It's okay if the resizing is slightly weird in three-pane mode, because it probably has to be, but it can't break it horribly.

I tried something like the approach in the patch in GTK 2 with the GtkTable infrastructure, and it did *not* work. However, it looks like GTK 3 and/or GtkGrid may have helped us out slightly.

While I'm probably okay with the interaction in this prototype, I would probably prefer to have a defined grab zone to resize panes... possibly in the 'header' as suggested in comment 18, in the dead space atop the linkmap widget. The grabby sometimes-rendering in the linkmap is offputting to me, as is having the cursor indicate resizability over the whole linkmap.

If you have specific questions, I'll try and address them. I'm sorry that there's no defined design for this feature, but it's just not 100% clear how it should look.
Comment 22 Adam Dingle 2014-03-20 20:30:21 UTC
Kai, thanks for your input.  All of your points sound reasonable.  By the way I posted the bounty, and I'm confident that if an implementation is good enough for Kai, it will be good enough for me.  :)
Comment 23 Marco Brito 2014-03-21 17:29:53 UTC
(In reply to comment #21)
> 
> My personal requirements (which may not be the same as the person who posted
> the bounty) are:
> 
>  * The UI should behave *exactly* as it does now, until the user tries to
> manipulate pane width. This includes things like getting default sizing right,
> etc. (which the current prototype doesn't)
With sizing, you mean the symmetrical same width textviews?
If we go with a custom container that will handle size allocation of its childrens, there should be no problem,

> 
>  * Visual impact on the current UI should be minimal. We'll use the same
> scrollbar placement, and linkmaps should still have a seamless join with
> textviews.
> 
>  * Three pane mode has to work. It's okay if the resizing is slightly weird in
> three-pane mode, because it probably has to be, but it can't break it horribly.
In two pane mode, will be same as in GtkPaned. In three panes, the two been resized will only affect the third pane size when the minimal size is reached.


> 
> I tried something like the approach in the patch in GTK 2 with the GtkTable
> infrastructure, and it did *not* work. However, it looks like GTK 3 and/or
> GtkGrid may have helped us out slightly.
> 
> While I'm probably okay with the interaction in this prototype, I would
> probably prefer to have a defined grab zone to resize panes... possibly in the
> 'header' as suggested in comment 18, in the dead space atop the linkmap widget.
> The grabby sometimes-rendering in the linkmap is offputting to me, as is having
> the cursor indicate resizability over the whole linkmap.
Yes i agree.


> 
> If you have specific questions, I'll try and address them. I'm sorry that
> there's no defined design for this feature, but it's just not 100% clear how it
> should look.
That is my proposit with this discussion, to find a good way.


Next, i will do another experiment with a derived of GtkGrid.
Comment 24 Kai Willadsen 2014-03-22 20:41:47 UTC
(In reply to comment #23)
> With sizing, you mean the symmetrical same width textviews?
> If we go with a custom container that will handle size allocation of its
> childrens, there should be no problem,

Yes, that's what I mean. I don't know whether a custom container is required; I couldn't get things to work without before, but that was a very different GTK with a totally different sizing infrastructure.

> In two pane mode, will be same as in GtkPaned. In three panes, the two been
> resized will only affect the third pane size when the minimal size is reached.

This isn't actually how your prototype worked, but I'm happy that the prototype behaviour wasn't too crazy anyway.

> Next, i will do another experiment with a derived of GtkGrid.

What are you trying to achieve with that? While I can imagine how that might help, I would have thought that doing some custom size handlers would probably do the job. That's just a hunch though!

Also, it's possible that a cleaned-up version of the previous patch would suffice. From memory, the grab zone and the default sizing were the only obvious issues with the subclassing-GtkPaned approach.
Comment 25 Marco Brito 2014-03-24 15:08:58 UTC
(In reply to comment #24)
> What are you trying to achieve with that? While I can imagine how that might
> help, I would have thought that doing some custom size handlers would probably
> do the job. That's just a hunch though!
Now that you ask, was just to see if there is a way to preserve the way of editing in glade editor. But is going nowhere.

> 
> Also, it's possible that a cleaned-up version of the previous patch would
> suffice. From memory, the grab zone and the default sizing were the only
> obvious issues with the subclassing-GtkPaned approach.
But, i don't like how the sizing is done, with a GtkPaned inside of another.
I was thinking of a gtkpaned with 3 panes, this requires to be a custom widget plus glade editor has to handle this.

I will think more about this.
Comment 26 Marco Brito 2014-03-25 14:24:35 UTC
Just to add.
That will likely take some days to weeks until i got something new to show. 
Meaning that there could be no comments from me, until then.
Comment 27 Marco Brito 2014-04-08 14:47:47 UTC
Created attachment 273807 [details] [review]
Experiment with custom size allocation, GtkGrid based.

Another experiment patch to trial the case of a custom widget handling the size allocation.
Was first aiming to be devired from GtkContainer, but when I arrived to add it to glade. I just couldn't find how to use a custom GladeWidgetAdaptor written in python. Then I went with GtkGrid, for first to make glade happy, second to preserve the visual look.

As the previous only the minimum was done, there are some bugs, in 2 panes window resizing and doesn't have any visual clue like changing the mouse cursor to double arrow.
Please only test in 3 panes mode, the resizing is done with mouse button 1 pressed and dragging in the area above the linkmaps.

Should i continue following this path?
Comment 28 Marco Brito 2014-04-08 14:51:39 UTC
Questions:

1. Glade integration is complicated with the use of python, i couldn't find how to use override GladeWidgetAdaptor functions that is need to tell glade were to put the widget placeholders. In alternative is the way of this patch, using a existent widget classes that in glade supports. Or break it apart, in glade don't add the widgets to the container but make the pane a template that is construted at runtime. Which is the preferred way?

2. This DiffGrid of last patch uses and 2x7 grid of widgets, it would simplify if it would only be 3 slots, similar to a gtkpaned with 3 panes, but the 3 question point would not be possible.

3. Currently the toolbar above panes, have some dummy toolbars in between to give the look of one continous toolbar, is this wanted to be preserved?

4. The area used for dragging will have the mouse cursor change to double arrow, but should also have any visual clue draw in?
Comment 29 Kai Willadsen 2014-04-12 21:03:03 UTC
(In reply to comment #27)
> Created an attachment (id=273807) [details] [review]
> Experiment with custom size allocation, GtkGrid based.

As a general style point, Meld tries to follow PEP8 for formatting. The patch is mostly okay with this... except for the tabs. Please use 4 spaces for indentation only.

> As the previous only the minimum was done, there are some bugs, in 2 panes
> window resizing and doesn't have any visual clue like changing the mouse cursor
> to double arrow.
> Please only test in 3 panes mode, the resizing is done with mouse button 1
> pressed and dragging in the area above the linkmaps.
> 
> Should i continue following this path?

So while the logic looks okay to me, I can't make this patch actually work. In both 2- and 3-way mode, I get no resizing when clicking and dragging the area about the linkmap; I tested both file and folder comparison.


(In reply to comment #28)
> Questions:
> 
> 1. Glade integration is complicated with the use of python, i couldn't find how
> to use override GladeWidgetAdaptor functions that is need to tell glade were to
> put the widget placeholders. In alternative is the way of this patch, using a
> existent widget classes that in glade supports. Or break it apart, in glade
> don't add the widgets to the container but make the pane a template that is
> construted at runtime. Which is the preferred way?

I can't really advise here, since I also haven't ever had to override the GladeWidgetAdaptor stuff. I'm okay with the way this patch does it.

> 2. This DiffGrid of last patch uses and 2x7 grid of widgets, it would simplify
> if it would only be 3 slots, similar to a gtkpaned with 3 panes, but the 3
> question point would not be possible.

I'm not sure what you mean here. Do you mean that the DiffMaps would be outside of the grid? That's certainly what we

> 3. Currently the toolbar above panes, have some dummy toolbars in between to
> give the look of one continous toolbar, is this wanted to be preserved?

Yes; they're there simply for styling so that we have a single clear separation between the secondary action areas and the panes. It's an ugly solution, but it works.

> 4. The area used for dragging will have the mouse cursor change to double
> arrow, but should also have any visual clue draw in?

It probably should, yes. A good place to start would be a centered grabby, styled like a standard GtkPaned (via gtk_paint_handle) would be my first guess.
Comment 30 Marco Brito 2014-04-14 19:57:58 UTC
(In reply to comment #29)
> 
> So while the logic looks okay to me, I can't make this patch actually work. In
> both 2- and 3-way mode, I get no resizing when clicking and dragging the area
> about the linkmap; I tested both file and folder comparison.
Maybe that is what is expected, because GtkToolBar doesn't have a GdkWindow itself. But is odd, i am getting events as is.


> > 2. This DiffGrid of last patch uses and 2x7 grid of widgets, it would simplify
> > if it would only be 3 slots, similar to a gtkpaned with 3 panes, but the 3
> > question point would not be possible.
> 
> I'm not sure what you mean here. Do you mean that the DiffMaps would be outside
> of the grid? That's certainly what we
Missing the end. Have?

Linkmap and diffmaps could be draw into this custom widget, instead of being a seperate child widget. Just another way of doing that could be preferred.
Comment 31 Marco Brito 2014-04-15 15:04:07 UTC
Created attachment 274374 [details] [review]
DiffGrid, experiment with custom size allocation.

Changelog from previous.
   Changed indentation to 4 spaces.
   Use GdkWindow for events, put over the child widget dummy toolbar in drag area.
   The cursor is set to double arrow.
   Draw visual clue in dragarea with Gtk.render_handle.
   Fix window resize in 2 panes mode.
   Fix the hidding of filechooser when Toolbar is set to minimum size.

Now is using two Gdkwindow to get the drag events, this is a bit trick in the way that is putting this windows in top of the child dummy toolbar. The do_map event is need to enforce the event window to be above, and same way the do_size_allocate needed to call show()

The logic of size allocation, and other details will be improved, as we see if this solution is good and should carry on.
Comment 32 Marco Brito 2014-04-28 10:04:26 UTC
Hi, can the last patch get feedback?

I prefer this going without hurries but if it had more exchanges it could go faster. And I understand that meld developers must be to busy to reply, maybe someother else could help keep the discussion going on.

I only trying to find a good solution that can be agreed by all. After that is when a complete patch will be written.
Comment 33 Kai Willadsen 2014-04-28 21:40:01 UTC
(In reply to comment #31)
> Changelog from previous.
<snip>
>    Draw visual clue in dragarea with Gtk.render_handle.

I'm not seeing this unfortunately. Maybe this is a theme-specific issue, but I'm using the default Adwaita theme, so we should definitely work with that.

> The logic of size allocation, and other details will be improved, as we see if
> this solution is good and should carry on.

I'm sad (though not surprised) by the amount of code required to do the custom container. I haven't had the time to actually review it in great detail, but so far it looks good.

I find it weird that as soon as we change the allocation of one of the child sections, we then treat extra allocation (from resizing the window) differently, and give it to the last pane. I think I'd prefer it if we maintained the other behaviour, and split new allocation between all panes. I'm not sure how tricky that is to do though.

We also do something similar with existing allocations when resizing panes in three-pane mode, in that we only change the allocation of the subsequent pane, not all other panes. I'm really not sure what to do here, as both behaviours can be a little weird at times.

It would be good if Adam could try out this version as well (applies cleanly to current HEAD)... he may have some feedback on the expected resize behaviour.

Finally, a few python style notes:
 * _drag_pos to _handle_window2 are all instance variables, so should be initialised in __init__, not at the class level
 * For line continuations (e.g., those in _new_drag_window), I prefer to use implicit continuation (i.e., surround the expression in parentheses) rather than backslash continuation.

Anyway, I think it's safe to say that something like this patch should work out. Nice work!
Comment 34 Marco Brito 2014-04-30 17:19:47 UTC
To make things easy, from now on when I need to have a reply to continue, I will make clean at the end of comment.


(In reply to comment #33)
> (In reply to comment #31)
> > Changelog from previous.
> <snip>
> >    Draw visual clue in dragarea with Gtk.render_handle.
> 
> I'm not seeing this unfortunately. Maybe this is a theme-specific issue, but
> I'm using the default Adwaita theme, so we should definitely work with that.
Odd. I am testing with the gtk3 themes that i could install, currently, Adwaita, Clearlooks-Phenix, High Contrast, Raleigh. Last two don't draw anything, but the first two do. This patch don't have, but the next I will add, change th state to PRELIGHT when the mouse is over.

> 
> > The logic of size allocation, and other details will be improved, as we see if
> > this solution is good and should carry on.
> 
> I'm sad (though not surprised) by the amount of code required to do the custom
> container. I haven't had the time to actually review it in great detail, but so
> far it looks good.
And this is only the minimum to work, and lucky the size request would be used from GtkGrid.


> 
> I find it weird that as soon as we change the allocation of one of the child
> sections, we then treat extra allocation (from resizing the window)
> differently, and give it to the last pane. I think I'd prefer it if we
> maintained the other behaviour, and split new allocation between all panes. I'm
> not sure how tricky that is to do though.
I am aware of that, proportionally resize the panes when resizing the window can make sense, but more in the next point.

> 
> We also do something similar with existing allocations when resizing panes in
> three-pane mode, in that we only change the allocation of the subsequent pane,
> not all other panes. I'm really not sure what to do here, as both behaviours
> can be a little weird at times.
If understood correctly. You mean when the separator handle1 is being dragged, only the pane1 and pane2 is resized, pane3 is not affected.
I writed specialy to have this effect.
Take as example gvim with multiple split views, it behaves this exact in this way.
The reason is, if the user resizes to a certain width, is because he want to be that width(for example to match width of the text that is viewed), if the resize of the window changes this width, the user will have to do again the resize of the pane.

> 
> It would be good if Adam could try out this version as well (applies cleanly to
> current HEAD)... he may have some feedback on the expected resize behaviour.
> 
> Finally, a few python style notes:
>  * _drag_pos to _handle_window2 are all instance variables, so should be
> initialised in __init__, not at the class level
>  * For line continuations (e.g., those in _new_drag_window), I prefer to use
> implicit continuation (i.e., surround the expression in parentheses) rather
> than backslash continuation.
Yes my bad, will make correction.

> 
> Anyway, I think it's safe to say that something like this patch should work
> out. Nice work!

Was done in mind to get minimal changes to existing code, but there is alot more that could be done.
1. Pane 3 mode is enabled and disabled by hiding and showing the individual widgets.
   DiffGrid can do this in method like, diffgrid.show_only_panes((1, 2, 3))

2. The dummies and toolbars could be removed, and instead this appearence 
   could be draw-in by Diffgrid

3. Same way linkmap and diffmap could be a draw function called by DiffGrid.


I will soon post a patch with the fixes.
Comment 35 Marco Brito 2014-05-01 14:15:31 UTC
Created attachment 275545 [details] [review]
DiffGrid, version 3.

Changes:
   Instance variables moved to __init__.
   Line continuations to use ().
   Set widget states in drag area, PRELIGHT, SELECTED.
   New class HandleWindow for drag area inplementation.
   Begining improving the size allocation logic.


I will continue next week, no need for a reply.
Comment 36 Adam Dingle 2014-05-02 23:12:29 UTC
I tried out this latest patch at Kai's suggestion.  It looks like it works pretty well! 

One note: I normally run Ubuntu with the default Ambiance theme.  In that theme, the resize handles between panes are not visible.  If I move my mouse into the header area between panes, the cursor changes to a double arrow and I am able to drag and resize.  But only when I switch to the Adwaita theme do I actually see the resize handles. 

I'll welcome this change when it's ready to land.
Comment 37 Marco Brito 2014-05-07 11:07:27 UTC
Is not visible because the background color is set to transparent in some themes.

https://git.gnome.org/browse/gtk+/tree/gtk/gtkthemingengine.c?h=gtk-3-12#n2507
Here, the lighter and darker colors are calculated from the background color, and if transparent the drawing is still made but with zero alpha.
Also if a background image is set nothing is draw.

Maybe we should go with a custom drawing here, independent of the theme used.
Our drag area is rectangle almost square area, when render_handle is used for  vertical or horizontal narrow lines.


No new patch this week.
Comment 38 Kai Willadsen 2014-05-11 20:25:52 UTC
I'm extremely busy at the moment, and have had almost no time to review the patch. However, I think it's looking good!

At this point, I need to find time go through and do a proper merge review to land it, but apart from that, the only things I think need addressing are:

 * We need to have a visible (though preferably subtle) grippy. It's weird that we're seeing such different theme results, but we need something. I'd suggest some simple drawing using either our custom CSS, or pulling theme colours. We can always adjust the drawing later.

 * The resizing behaviour needs tweaking (as below)

(In reply to comment #34)
> > I find it weird that as soon as we change the allocation of one of the child
> > sections, we then treat extra allocation (from resizing the window)
> > differently, and give it to the last pane. I think I'd prefer it if we
> > maintained the other behaviour, and split new allocation between all panes. I'm
> > not sure how tricky that is to do though.
> I am aware of that, proportionally resize the panes when resizing the window
> can make sense, but more in the next point.

So playing with this more, I'm feeling pretty insistent that this is how it should work. I can go either way on the behaviour when you're dragging the separators; what you have now is fine. However, I think that any extra allocation (or reduced allocation) from resizing the window itself *must* be shared between the panes.

> Was done in mind to get minimal changes to existing code, but there is alot
> more that could be done.
> 1. Pane 3 mode is enabled and disabled by hiding and showing the individual
> widgets.
>    DiffGrid can do this in method like, diffgrid.show_only_panes((1, 2, 3))

Yeah this is an idea that's been around. I'm interested, though personally I've never actually wanted to turn a two-way diff into a three-way one. Still, it's definitely worth considering.

> 2. The dummies and toolbars could be removed, and instead this appearence 
>    could be draw-in by Diffgrid

That's a possibility. In theory the current setup should look consistent across themes, which custom drawing can't easily do. In reality, the default Ubuntu theme at least does something awful with toolbar outlines, so I'm not sure what's better here.

> 3. Same way linkmap and diffmap could be a draw function called by DiffGrid.

For these, I'd prefer to keep them as-is. Meld used to have linkmap and diffmap all bundled in to the main filediff module, and then duplicated into dirdiff... this way is better. :)
Comment 39 Marco Brito 2014-05-20 12:13:31 UTC
(In reply to comment #38)
> I'm extremely busy at the moment, and have had almost no time to review the
> patch. However, I think it's looking good!
> 
> At this point, I need to find time go through and do a proper merge review to
> land it, but apart from that, the only things I think need addressing are:
No hurries. I also couldn't find time to work in last week. For now i would only like to have help in the usability aspect and what and not feature it should have.
After this, i will clearly say that is ready for reviewing.

> 
>  * We need to have a visible (though preferably subtle) grippy. It's weird that
> we're seeing such different theme results, but we need something. I'd suggest
> some simple drawing using either our custom CSS, or pulling theme colours. We
> can always adjust the drawing later.
I think this is the correct behavior, some themes + engines like Raleigh by design doesn't draw a grippy. And it can be seen in the GtkPaned also doesn't have anything draw.

> 
>  * The resizing behaviour needs tweaking (as below)
Done in the next patch.
Comment 40 Marco Brito 2014-05-20 12:29:00 UTC
Created attachment 276854 [details] [review]
DiffGrid, version 4.

Changes:
   When resizing the widget, the panes are proportional resized.
   If drag area background is transparent, draw a vertical line.
   Some fixes in code structure.
Comment 41 Kai Willadsen 2014-05-24 21:28:12 UTC
Review of attachment 276854 [details] [review]:

I'm basically ready to land this. I'm happy with the behaviour, and I like that everything is nicely self-contained so that existing modules aren't affected. I have a few minor review quibbles, but basically this is good.

I am still not seeing drag handles in the top pane, but I've tracked this down to Gtk.cairo_should_draw_window() returning False... I have no idea why, but either way that's not a prerequisite for merging these changes.

I'm hoping to put out 3.11.1 *very* soon, and I'll be happy to merge the final patch directly after that.

::: meld/diffgrid.py
@@ +35,3 @@
+        self._handle2.realize(self)
+
+

Here and at many places below... can I ask you to just run a PEP8 checker over this file? There should be single lines between function definitions at the class level. Also, there are a few lines that exceed 80 characters.

@@ +150,3 @@
+        self._handle1.set_position(pos1)
+        self._handle2.set_position(pos2)
+        return round(pos1), round(pos2)

While I don't expect it to break anything, round() will return a float, and since these are pixel coordinates you're probably expecting integers?

::: meld/ui/catalog.xml
@@ +8,3 @@
+                <property id="n-rows" default="2" query="False"/>
+                <property id="n-columns" default="7" query="False"/>
+            </properties>

These properties look like they were planned-for, but not actually used? I'm okay with us not having them (though they'd be nice), but these properties should be removed here if not supported.
Comment 42 Marco Brito 2014-05-27 16:20:23 UTC
(In reply to comment #41)
> Review of attachment 276854 [details] [review]:
> 
> I'm basically ready to land this. I'm happy with the behaviour, and I like that
> everything is nicely self-contained so that existing modules aren't affected. I
> have a few minor review quibbles, but basically this is good.
> 
> I am still not seeing drag handles in the top pane, but I've tracked this down
> to Gtk.cairo_should_draw_window() returning False... I have no idea why, but
> either way that's not a prerequisite for merging these changes.
I am also clueless, that function only does matches if the window is the same window of the cairocontext.
What is your window manager and library versions, i would like to try to reproduce this bug.

 
> Here and at many places below... can I ask you to just run a PEP8 checker over
> this file? There should be single lines between function definitions at the
> class level. Also, there are a few lines that exceed 80 characters.
Done.

> @@ +150,3 @@
> +        self._handle1.set_position(pos1)
> +        self._handle2.set_position(pos2)
> +        return round(pos1), round(pos2)
> 
> While I don't expect it to break anything, round() will return a float, and
> since these are pixel coordinates you're probably expecting integers?
This round is to preserve the symmetry in the panes width, without it there is a by 1 pixel shift.

 
> ::: meld/ui/catalog.xml
> @@ +8,3 @@
> +                <property id="n-rows" default="2" query="False"/>
> +                <property id="n-columns" default="7" query="False"/>
> +            </properties>
> 
> These properties look like they were planned-for, but not actually used? I'm
> okay with us not having them (though they'd be nice), but these properties
> should be removed here if not supported.
They are only for the integration of this custom widget in the glade editor.
When creating a new DiffGrid to use this defaults and to not query the user. Is only to not show that window askings for the number of rows/columns when a GtkGrid is created.
Comment 43 Marco Brito 2014-05-27 16:42:22 UTC
Created attachment 277321 [details] [review]
DiffGrid, version 5.

Changes:
   Fix style conventions to respect PEP8 checker.
   Use int(round()) to convert from float positions to integer.
   More fixes in code structure.

The size allocation logic is not the best, and I will try to rewrite it better doing this next week.
I don't think is ready to be merged and commited to meld repository yet.
So please wait until them.
Comment 44 Marco Brito 2014-05-31 12:41:37 UTC
Created attachment 277619 [details] [review]
DiffGrid, version 6.

Changes:
   New resize logic for any panes hidden or showing.
Comment 45 Kai Willadsen 2014-05-31 22:21:02 UTC
In your opinion, is this ready to be merged?

If so, can I ask you to please attach a final patch in git format-patch style, including a commit message explaining what the changes are, etc. I haven't tried the most recent patch yet, but as I said earlier, I'm pretty happy to have this go in.
Comment 46 Marco Brito 2014-06-04 14:38:28 UTC
Created attachment 277882 [details] [review]
DiffGrid, version 7.

Changes:
   Remake last patch with git format-patch


(In reply to comment #45)
> In your opinion, is this ready to be merged?
Well, there is allways something that could be rewritten in other way.
But i think is ready, or at least i am in the point of not knowing what else could be done.
Comment 47 Kai Willadsen 2014-06-07 22:42:23 UTC
(In reply to comment #46)
> (In reply to comment #45)
> > In your opinion, is this ready to be merged?
> Well, there is allways something that could be rewritten in other way.
> But i think is ready, or at least i am in the point of not knowing what else
> could be done.

All good. I've merged this patch to current master.

I'm still a little wary of the theme drawing issues, but I'll try to chase those down when I have time. As it stands, this is nicely self-contained, and seems to me to work well, so I'm happy to call this done. Thanks for the patch!
Comment 48 Marco Brito 2014-06-09 21:15:26 UTC
Understood.
If anything shows up, CC list me and i will look at it.