Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 107197 - [Forms] MasterDetailsBlock should not make assumptions on the way it should be laid out.
Summary: [Forms] MasterDetailsBlock should not make assumptions on the way it should b...
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: User Assistance (show other bugs)
Version: 3.1   Edit
Hardware: PC Windows XP
: P3 enhancement with 2 votes (vote)
Target Milestone: 3.5 M5   Edit
Assignee: Adam Archer CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-08-17 08:08 EDT by Roland Tepp CLA
Modified: 2009-01-25 21:58 EST (History)
6 users (show)

See Also:


Attachments
My proposed patch to fix MasterDetailsBlock (4.27 KB, patch)
2005-08-17 08:12 EDT, Roland Tepp CLA
no flags Details | Diff
Backwards compatible patch to MasterDetailsBlock (5.59 KB, patch)
2008-08-27 08:05 EDT, Roland Tepp CLA
agarcher: iplog+
Details | Diff
Modified version of Roland's patch (4.10 KB, patch)
2009-01-25 21:55 EST, Adam Archer CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Tepp CLA 2005-08-17 08:08:27 EDT
Current implementation of the MasterDetails block assumes it is the only item on
the managed form and sets itself up with a GridLayout and accompanying GridData
on the body of that form.

As a component of the form it should not make any such assumptions and allow
itself to be positioned by the author of the form. Also it should not interfere
with the form layout and subsequently it should not apply layout data on
underlying sashForm.
Comment 1 Roland Tepp CLA 2005-08-17 08:12:22 EDT
Created attachment 26191 [details]
My proposed patch to fix MasterDetailsBlock

The proposed changes in short are following:
* Remove the layout creation and assignment from createContent method.
* Remove the layout data assignment to sashForm from createContent.
* Methods to get/set layout data to sashForm outside the MasterDetailsBlock
* Added new implementation of createContent method allowing to set parent
composite for sashForm
Comment 2 Dejan Glozic CLA 2007-01-25 17:52:04 EST
This is an enhancement request.
Comment 3 Eric Rizzo CLA 2008-05-01 10:58:20 EDT
I argue that this is not clearly an enhancement because MDB is broken if you try to put it on a page with other layout/controls.
In addition to the overall complaint, MDB sets the layout margins to 0, which looks ugly and is inconsistent with most forms-based pages.
Since there is a patch, what will it take to get this addressed? The problems with MDB have been logged in several bugs but AFAICT none of them has been properly addressed, much less fixed.
Comment 4 Adam Archer CLA 2008-05-01 11:07:02 EDT
I tend to agree that the MDB should not presume that it makes up the entire managed form. The problem with addressing this though, is that this has been the behaviour for a very long time and there are many people depending on it at this point. We certainly cannot change the default behaviour of the class as it would break too many people. The only way we could address this, that I can think of, would be to add API to allow people to opt-out of the automatic layouting, but we are well past the API freeze for 3.4.
Comment 5 Eric Rizzo CLA 2008-05-01 11:32:54 EDT
Preface: this is going to sound confrontational,; hopefully nobody will take it personally.
I find it increasingly frustrating that we have a component that is clearly broken but can't find the political will to fix it for fear of "breaking" something that relies on the broken behavior. I understand that position, up to a point, but there are numerous problems with MDB that can't be addressed as long as that is the ruling philosophy.
I'm not even sure I see how an app could be depending on the layout behavior in a way that would "break" it if this were corrected, but I also think MDB could be modified to respect it's parent's existing layout if one has been set.
Also, API change is not necessarily a bad thing; in fact, it is only necessary here because of the self-imposed prohibition on breaking current behavior (behavior that is agreed to be incorrect). It seems to me that you can't have it both ways: if an strict attitude of not jeopardizing current behavior is insisted upon, you have to be willing to accept additional API to allow for refinement and correction. It's got to be one or the other, not both (as far as I see it).
I asked about this before 3.3 and was told it was too late; now I'm told its too late for 3.4. Do I hold any hope for 3.5? Not really. It seems MDB is the step-child that nobody cares about fixing :-) Just look at how old all these bugs are.

So do I have anything to contribute other than complaints? Yes, in fact; an idea: Can we get a new component that fixes the problems with MDB, and deprecate MDB going forward? I could even see such a new thing taking advantage of Data Binding, if such a dependency is acceptable. Is that even an idea that would be accepted? (before anyone spends time contributing some code, it is good to know that the idea has legs). I don't personally have time right now to contribute such a thing, but maybe someone already involved in the Forms plugin would be interested in fixing this and being a hero to many...
Comment 6 Dejan Glozic CLA 2008-05-01 16:41:43 EDT
MDB is a prepackaged useful part that is created for a well defined use case - adding it to a multi-page editor where it consumes an entire page (and therefore is in the root of the form). It is not a bug that it is not more versatile - it has been created with a more narrow focus but with an intent to work well right away. When you think of it, it is not very hard to make your own - it is just two viewers where selections in one drive another. Therefore I don't see how having needs that make MDB unsuitable for the task make MDB less useful.

Eclipse prides itself for its backward compatibility whenever possible. We see API contracts seriously and I can only see changing this if we add API that overrides the default behaviour (even if you find it wrong). The only thing that would be wrong is if people all of the sudden found that they need to fix their code that worked great in the previous version. Forward progress must not be paid by breaking backward compatibility, at least not in the minor release increment.
Comment 7 Benjamin Bihler CLA 2008-08-27 02:38:11 EDT
I have spent many hours trying to insert a MasterDetails block into an FormPage together with other elements until I have been told in the newsgroup eclipse.platform.rcp, that the MDB is not designed to be used in this way.

For me this seems to be definately a bug, but if this will not be changed, it should at least be mentioned in the documentation!!!

The paragraph

Platform Plug-in Developer Guide > Programmer's Guide > UI Forms > Advanced Topics > Master/Details block

in the Eclipse documentation does not clearly state that.
Comment 8 Roland Tepp CLA 2008-08-27 08:05:24 EDT
Created attachment 111058 [details]
Backwards compatible patch to MasterDetailsBlock

Here is another version of the patch that retains backwards compatibility with clients that use current implementation, but still allows implementors to override critical layout related stuff...

* MasterDetailsBlock can be placed on any composite, not necessarily directly on ManagedForm
* Layout of the MDB placement can be overriden
Comment 9 Roland Tepp CLA 2008-08-27 08:07:12 EDT
Comment on attachment 26191 [details]
My proposed patch to fix MasterDetailsBlock

The old patch is now obsolete
Comment 10 Eric Rizzo CLA 2009-01-12 08:52:56 EST
Has any committer bothered to look at the patch that Roland submitted more that 4 months ago? It claims to maintain backwards compatibility while adding new flexibility via API.
The point of any Eclipse project is to serve the community; in this case there is a common request that has been discussed and a patch submitted. Yet there is no action taken by the project committers. Are we doomed to another "we're past API freeze" response? I certainly hope not.
Comment 11 Benjamin Bihler CLA 2009-01-13 04:15:06 EST
(In reply to comment #10)
> Has any committer bothered to look at the patch that Roland submitted more that
> 4 months ago? It claims to maintain backwards compatibility while adding new
> flexibility via API.

I am using the patched MasterDetailsBlock. It works great as far as the mentioned problem is concerned: now I can put more than one MasterDetailsBlocks on one form page.

There is only one problem left: the layout. I did not manage to put three master details blocks into a form page with one section so that they fill the width of the page. My master parts contain one table each while the details parts contain text fields. Unfortunately I am not very experienced with layouts and I had to work on my project under a high pressure of time. So I don't know whether this is a problem with the patched MDB or whether I was just too stupid to get it working. My workaround was to set the width of the table in the master block manually. Now the MDBs do not resize when the window is resized, but they are useable.
Comment 12 Adam Archer CLA 2009-01-13 13:57:04 EST
I took a quick look at the patch and it looks quite promising. I will find some time to do some testing shortly and will consider it for M5.
Comment 13 Adam Archer CLA 2009-01-25 21:55:03 EST
Created attachment 123699 [details]
Modified version of Roland's patch

I have reviewed Roland's patch and in general it looks good. I have made a couple changes and delivered it to HEAD for M5.

I removed the setLayoutData and getLayoutData methods as it should be enough to override the applyLayout and applyLayoutData methods and this reduces the commitment to new API. If these methods are really needed by consumers they can be added in the subclass anyway.

Thanks for the contribution Roland!
Comment 14 Adam Archer CLA 2009-01-25 21:58:38 EST
Resolving as FIXED.