364572 - [Proto] Unable ro resize the transparent overlay in free/Busy grid

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Omar Bajraszewski]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

When I narrowed the transparent overlay to minimum ( it looked like a line) it was impossible to widen it

Reproducible: Always

Steps to Reproduce:
1.Open "invite attendees" dialog
2.Narrow the transparent overlay as much as possible
3.Try to widen it

or

1.Mark an avent as all day event
2.Open Invite Attendees dialog
3.Uncheck all day box in Information and Date/Time Area
4.Try to widen the transparent overlay




Lightning 2006122004

Comment 1

[Martin Schröder [:mschroeder]]

Can you reproduce this bug with latest nightly builds of Lightning? Can you attach screenshots that show your problem?

Comment 2

[Omar Bajraszewski]

Prototype has been updated- I cannot reproduce it with latest build. I'm marking the bug as WFM

Comment 3

[Omar Bajraszewski]

I can see the same problem again with today (26/02/2007) build. Can I reopen bug?

Comment 4

[Martin Schröder [:mschroeder]]

Please attach a screenshot if possible.

Comment 5

[Stefan Sitter [:ssitter]]

(In reply to comment #3)
Please always mention the product + version + BuildID you use.

Comment 6

[Omar Bajraszewski]

Attachment: Unresizeable transparent overlay

I can't widen the blue line

Comment 7

[Omar Bajraszewski]

Attachment: Flash movie

Comment 8

[Omar Bajraszewski]

Lightning 0.5pre 2007022603
WCAP Enabler 0.3 20070226
Thunderbird version 2.0pre (20070226)

As mentioned in comment #3

Comment 9

[Michael Büttner (no reviews TFN)]

omar, many thanks for the movie, this was a great help ;-) taking this one...

Comment 10

[Berend Cornelius]

bc: I will take over.

Comment 11

[Berend Cornelius]

bc: I could also reproduce on Solaris

Comment 12

[Berend Cornelius]

Attachment: patch to query the startdate or enddate

berend: I added a check that queries if the startdate will exceed the enddate or if the enddate is smaller than the start date

Comment 13

[Michael Büttner (no reviews TFN)]

Comment on attachment 258301 [details] [diff] [review]
patch to query the startdate or enddate

>+	  var element = event.target;
>+	  var mouseX = event.clientX - element.boxObject.x;
>+	  var mouseY = event.clientY - element.boxObject.y;
>+          var delta = mouseX - this.mMouseX;
Obviously you moved those lines out of the separate if-statements in order to removed redundancy, that's fine for me. Thanks for spotting this issue.

>+//	    var cs = Components.classes["@mozilla.org/consoleservice;1"].getService(Components.interfaces.nsIConsoleService);
>+//	    cs.logStringMessage(newEnd.toString()); // + " " + this.mEndDate.toString() + " " + this.startDate.toString());	    
Please remove this, since we don't want to have loads of debugging code here.

>+              if((!newEnd.isDate) || (newEnd.compare(this.startDate) 
Please keep the original intendation, since it's not necessary to touch this line.

There're are several lines that add unnecessary spaces, could you please removed those? And most probably, you didn't intend to include the garbage at the end of the patch, could you please remove this?

Comment 14

[Berend Cornelius]

Attachment: corrected patch

berend. I overworked the patch

Comment 15

[Berend Cornelius]

Attachment: corrected patch

berend. I overworked the patch

Comment 16

[Berend Cornelius]

Attachment: corrected patch

berend. I overworked the patch

Comment 17

[Michael Büttner (no reviews TFN)]

Comment on attachment 258304 [details]
corrected patch

I think this patch has been submitted twice, marking obsolete.

Comment 18

[Michael Büttner (no reviews TFN)]

Comment on attachment 258305 [details] [diff] [review]
corrected patch

>+	  var element = event.target;
>+	  var mouseX = event.clientX - element.boxObject.x;
>+	  var mouseY = event.clientY - element.boxObject.y;
>+          var delta = mouseX - this.mMouseX;
There are tabs at the start of these lines...

>+              // an all-day event. This is because setting 'endDate'
I'd prefer keeping the comment lowercase, this was intentional.

>+	    if (newStart.compare(this.mEndDate) >= 0) {
>+	    	return;
>+	    }
Again, there are tabs at the start of these lines...

>+	    if (newEnd.compare(this.mStartDate) <= 0) {
>+	    	return;
>+	    }
Same here...

>+              if(!newEnd.isDate || 
Here's a new space been added to the end of the line

>                  (newEnd.compare(this.startDate) >= 0)) {
>-
>+		 
And here are several tabs and spaces in this line...

First of all, it is no longer possible to shrink the duration of the event  below a certain amount of time. Basically I'm fine with this approach, but the fact that the limit depends on the current zoom-factor strikes me as being inconsistent. I'd like Christian to comment on this, if he supports this approach I'll shut up.

Unfortunately, I found another caveat with this patch. Setting the start- and end-time to the same time brings the overlay down to a thin strip (as expected). But from there on, it's now impossible to drag either the left or right side of it to bring it back into shape. Thus marking this as r-

Comment 19

[Berend Cornelius]

I resolved the issue about the impossibility to widen the overlay from a small strip in the patch that I attach. For the remaining problem I will wait for feedback from Christian

Comment 20

[Berend Cornelius]

Attachment: preliminary patch

Comment 21

[Berend Cornelius]

Attachment: The new patch

I fixed the remaining issue. I reworked the method "moveTime" because I found it a bit because I found it was not so well layouted. One flaw in this respect was that the overlay did always follow the mousepointer in an inert way. when it is moved or resized. I encountered this by defining the method "isTimeTobeClipped" as you can see. I am not sure if this requires a UI review.

Comment 22

[Michael Büttner (no reviews TFN)]

Comment on attachment 264254 [details] [diff] [review]
The new patch

>+!�;66933;66933
>+!�;71013;71013
I'd be glad if we could get rid of this garbage...

>+!�;47970;47970
>+!�;48826;48826
>+!�;52134;52134
>+!�;81233;81233
Same here...

>+      <method name="isTimeTobeClipped">
>         <parameter name="delta"/>
>+        <parameter name="clip_minutes"/>
I don't understand why we need a separate new method here that is completely internal and gets called just once. Furthermore, the second argument is not necessary since it's just "60 * this.zoomFactor / 100".

>+            return (Math.abs(minute_shift) >= (0.8 * clip_minutes))
Is there any reason for introducing magic numbers?

>+            if(this.mMouseX <= (this.mMargin+this.mWidth+7)) {
More magic numbers?

Marking r- since it's still possible to drag the start-date of an all-day event beyond the end-date.

Comment 23

[Berend Cornelius]

Attachment: The very new patch

I fixed the remaining bug about the invalid enddate when its an all-day event.

Comment 24

[Michael Büttner (no reviews TFN)]

Comment on attachment 265653 [details] [diff] [review]
The very new patch

>+          <xul:box anonid="leftbox" width="3" style="cursor: w-resize"/>
>+          <xul:box anonid="rightbox" width="3" style="cursor: e-resize"/>
I think we don't want to differentiate between left and right side in terms of which cursor is used.

>+      <field name="mLeftBoxWidth">3</field>
>+      <field name="mRightBoxWidth">3</field>
It would be much better to specify a css rule for the width of the drag-boxes and dynamically query for this attribute. I'm well aware that I didn't do it that way, but if we're touching this area there's no reason why we shouldn't fix it. These fields are not necessary, but feel free to keep them. But please don't initialize them with magic numbers.

>+          var oElement = document.getAnonymousElementByAttribute(this, "anonid", "leftbox");
>+          this.mLeftBoxWidth = Number(oElement.getAttribute("width"));
>+          var oElement = document.getAnonymousElementByAttribute(this, "anonid", "rightbox");
>+          this.mRightBoxWidth = Number(oElement.getAttribute("width"));
you're initializing the oElement variable twice. besides that, you don't need to keep the element, all you're interested in is the width.

>+          this.mSelectionbarBox = document.getAnonymousElementByAttribute(this, "anonid", "selection-bar").boxObject;
It's probably better to keep the dom element itself instead of the boxobject. You can always step from one to the other, but not visa versa.

>             this.mWidth = duration_in_hours*hour_width;
>+            if (this.mWidth == 0){
>+                this.mWidth = this.mRightBoxWidth + this.mLeftBoxWidth;
>+            }
This is what actually was missing and caused the trouble. It doesn't make a hell of a difference, but wouldn't it be better to clip at mRightBoxWidth+mLeftBoxWidth? In other words, if mWidth is less than this value, set it.

>+      <method name="isTimeTobeClipped">
I still don't like this newly introduced method, since I don't see a reason for it. But if you think it makes the code more readable, just go for it.

>+            const fClipRatio = 0.7;
More magic numbers. At least move this to a property or field and write a comment on what this is intended for. Probably the best way would be to model this as an attribute for the selection bar binding.

>+            return (Math.abs(minute_shift) >= (fClipRatio * clip_minutes))
Missing semicolon.

>+                if(delta > 0){
Missing space.

>+                    if(time.isDate) {
Missing space.

>+                else if(delta < 0) {
Missing space.

>+                    if(time.isDate) {
Missing space.

>-              }
>+                newTime.normalize();
Wrong indendation.

>             
>             newTime.normalize();
>-                        
>+
>             return newTime;
^^^^^^ This line didn't change, did it?

>+            if(this.mMouseX <= (this.mMargin + this.mWidth)) {
>+              // move the startdate and the enddate
Missing space.

>               this.mDragState = 1;
>               window.setCursor("-moz-grab");
>-              if(this.mMouseX <= (this.mMargin+3)) {
>-                window.setCursor("e-resize");
>+              if(this.mMouseX <= (this.mMargin + this.mLeftBoxWidth)) {
>+                // move the startdate only...
Missing space.

>+                window.setCursor("w-resize");
>                 this.mDragState = 2;
>-              } else if(this.mMouseX >= (this.mMargin+this.mWidth-3)) {
>+              } else if(this.mMouseX >= (this.mMargin+this.mWidth-this.mRightBoxWidth)) {
>+                // move the enddate only..
Missing space.

>+            if (newStart.compare(this.mEndDate) >= 0) {
>+                if (!this.mStartDate.isDate){
Missing space before the opening curly brace.

Besides these comments, I observed that the selection bar jumps a bit to the right in case you're trying to drag it to the left. This happens for one or two hours, after that it seems to work. This is indeed introduced with this patch, I tried it without and everything works fine.

Another issue is with what this patch is really all about. Just collapse the duration of the event in order to make the selection bar as small as possible. Then try to drag the right side of the small bar to the right, it works. Nothing happens if you try to move to the left. The same holds true for the other way 'round. It obvious why it happens, you're not allowed to drag the right drag-bar farther to the left as the left bar dictates, and visa versa. In this case it's a bit unfortunate.

Comment 25

[Berend Cornelius]

Attachment: The patch that resolves the remaining issue

I resolved the remaining issue.

Comment 26

[Berend Cornelius]

Attachment: a new patch...

Found another issue that I fixed within this patch

Comment 27

[Michael Büttner (no reviews TFN)]

Comment on attachment 265796 [details] [diff] [review]
a new patch...

>+          <xul:box anonid="leftbox" width="3" style="cursor: w-resize"/>
>+          <xul:box anonid="rightbox" width="3" style="cursor: e-resize"/>
First, could you please file a follow-up bug and ask christian what exactly the desired behavior we want to have? Do we want to drag one side up to the other one but no further? If that's the way we want to go, I'm fine with it. Otherwise we'd want to have just a resize cursor here and freely drag the sides of the slider.

Second, if you don't mind, please move the theme dependent stuff to *.css files. Please see my previous comments.

>+      <!-- constant that defines at which ratio an event is clipped, when moved or resized -->
>+      <field name="mfClipRatio">0.7</field>
I'd prefer to make this an inherited attribute that gets specified by the parent binding. Please see my previous comments.

>+      <field name="mSelectionbarBox"/>
This should be 'mSelectionBar'. Please see my previous comments.

>+          var oElement = document.getAnonymousElementByAttribute(this, "anonid", "leftbox");
>+          this.mLeftBoxWidth = Number(oElement.getAttribute("width"));
>+          oElement = document.getAnonymousElementByAttribute(this, "anonid", "rightbox");
>+          this.mRightBoxWidth = Number(oElement.getAttribute("width"));
>+          this.mSelectionbarBox = document.getAnonymousElementByAttribute(this, "anonid", "selection-bar").boxObject;
'o' is no commonly used prefix, please just name this 'element'. Also, I'd prefer to keep the dom element itself, not its boxObject. Please see my previous comments.

>+            if (this.mWidth < this.mRightBoxWidth + this.mLeftBoxWidth){
>+                this.mWidth = this.mRightBoxWidth + this.mLeftBoxWidth;
>+            }
There's a missing blank before the opening brace. I also suggest keeping right+left in a local variable. Otherwise this expression gets evaluated twice.

>+      <method name="moveTime">
>+        <parameter name="time"/>
>+        <parameter name="delta"/>
>+        <parameter name="bdoclip"/>
>+        <body>
>+          <![CDATA[
we don't use hungarian notation, so please get rid of those prefixes. also, please move 'isTimeTobeClipped()' back to this method, I still don't see a need for making this a separate functions.

>+            var brecalculate = true;
it's not necessary to introduce this variable at all.

>+            brecalculate = this.isTimeTobeClipped(delta, clip_minutes);
>+            if (brecalculate) {
please inline isTimeTobeClipped() and get rid of this variable.

>       <handler event="mousemove">
>         <![CDATA[
>-          if(this.mDragState == 1) {
>-            var element = event.target;
>-            var mouseX = event.clientX - element.boxObject.x;
>-            var mouseY = event.clientY - element.boxObject.y;
>+          var mouseX = event.screenX;
>+          if (this.mDragState == 1) {
>+            // move the startdate and the enddate
>             var delta = mouseX - this.mMouseX;
>-            var newStart = this.moveTime(this.mStartDate,delta);
>-            if(newStart.compare(this.mStartDate) != 0) {
>-              newEnd = this.moveTime(this.mEndDate,delta);
>+            var newStart = this.moveTime(this.mStartDate, delta, false);
>+            if (newStart.compare(this.mStartDate) != 0) {
>+              newEnd = this.moveTime(this.mEndDate, delta, false);
What exactly is the reason for not passing 'true' to moveTime(startTime) as clipped argument?

Generally, please adhere to the '1 tab == 2 blanks' rule (most notably moveTime()).

Comment 28

[Berend Cornelius]

(In Repyly to the last comment)
>>+          <xul:box anonid="leftbox" width="3" style="cursor: w-resize"/>
>>+          <xul:box anonid="rightbox" width="3" style="cursor: e-resize"/>
>First, could you please file a follow-up bug and ask christian what exactly the
>desired behavior we want to have? Do we want to drag one side up to the other
>one but no further? If that's the way we want to go, I'm fine with it.
>Otherwise we'd want to have just a resize cursor here and freely drag the sides
>of the slider.

Concerning the mentioned drag behaviour of the selection-bar I submitted https://bugzilla.mozilla.org/show_bug.cgi?id=382603. Regarding the used resize cursor some notes:
Alternatively to the used 'w-resize' and 'e-resize" cursors I could use a "col-resize" cursor, but as I checked the use of direction dependent cursors is very commonly used in Solaris. Also in Thunderbird such a cursor is frequently used for example when a splitter is moved by the user. A typical use for a 'col-resize' cursor is when the width of column headers in a spreadsheet document are to be changed, but this is a different usecase compared to this issue. In the windows OS the display of resize cursors is direction independent and always the same. Therefor I think we should stick to current cursors of this patch.

Comment 29

[Berend Cornelius]

(In reply to the following comment)
> >+      <!-- constant that defines at which ratio an event is clipped, when >moved or resized -->
>>+      <field name="mfClipRatio">0.7</field>
>I'd prefer to make this an inherited attribute that gets specified by the
>parent binding. Please see my previous comments.

Is it really a good idea to move such a relatively nonsignificant attribute to the parent binding. Why then not also move the "ZoomFactor", the "basedate", "range"... . Besides that I also have in mind that one day we might want to have two clipratios: one for allday-events and one for the other events. I would like to keep the clipratio at its current place where all other User interface related variables are declared.

Comment 30

[Berend Cornelius]

Attachment: final patch

(In replay to comment #27)
>>       <handler event="mousemove">
>>         <![CDATA[
>>-          if(this.mDragState == 1) {
>>-            var element = event.target;
>>-            var mouseX = event.clientX - element.boxObject.x;
>>-            var mouseY = event.clientY - element.boxObject.y;
>>+          var mouseX = event.screenX;
>>+          if (this.mDragState == 1) {
>>+            // move the startdate and the enddate
>>             var delta = mouseX - this.mMouseX;
>>-            var newStart = this.moveTime(this.mStartDate,delta);
>>-            if(newStart.compare(this.mStartDate) != 0) {
>>-              newEnd = this.moveTime(this.mEndDate,delta);
>>+            var newStart = this.moveTime(this.mStartDate, delta, false);
>>+            if (newStart.compare(this.mStartDate) != 0) {
>>+              newEnd = this.moveTime(this.mEndDate, delta, false);
>What exactly is the reason for not passing 'true' to moveTime(startTime) as
>clipped argument?

When the whole selection-bar is moved to another place within the scrollbox (DragState == 1) I took care that the range between startdate and enddate remains the same. This was not always the case beforehand: When one of them occured at a time that cannot be diveded by the zoom dependent "clipping" time, e.g 08:25AM.

Comment 31

[Michael Büttner (no reviews TFN)]

Comment on attachment 266905 [details] [diff] [review]
final patch

Everything looks fine, thanks for the patch. r=mickey.

Comment 32

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 33

[Omar Bajraszewski]

Verified with Lightning 2007070403 and lightning-wcap (20070704)