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

VERIFIED FIXED in 0.7

Status

VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: omarb.public, Assigned: berend.cornelius09)

Tracking

unspecified

Details

Attachments

(3 attachments, 9 obsolete attachments)

(Reporter)

Description

12 years ago
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
Summary: Unable ro resize the transparent overlay in free/Busy grid → [Proto] Unable ro resize the transparent overlay in free/Busy grid
Can you reproduce this bug with latest nightly builds of Lightning? Can you attach screenshots that show your problem?
(Reporter)

Comment 2

12 years ago
Prototype has been updated- I cannot reproduce it with latest build. I'm marking the bug as WFM
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 3

12 years ago
I can see the same problem again with today (26/02/2007) build. Can I reopen bug?
Please attach a screenshot if possible.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
(In reply to comment #3)
Please always mention the product + version + BuildID you use.
(Reporter)

Comment 6

12 years ago
Created attachment 256510 [details]
Unresizeable transparent overlay

I can't widen the blue line
(Reporter)

Comment 7

12 years ago
Created attachment 256513 [details]
Flash movie
(Reporter)

Comment 8

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

As mentioned in comment #3
omar, many thanks for the movie, this was a great help ;-) taking this one...
Assignee: nobody → michael.buettner
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 10

12 years ago
bc: I will take over.
Assignee: michael.buettner → Berend.Cornelius
(Assignee)

Comment 11

12 years ago
bc: I could also reproduce on Solaris
OS: Windows XP → All
(Assignee)

Comment 12

12 years ago
Created attachment 258301 [details] [diff] [review]
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
Attachment #258301 - Flags: first-review?(michael.buettner)
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?
Attachment #258301 - Flags: first-review?(michael.buettner) → first-review-
(Assignee)

Comment 14

12 years ago
Created attachment 258303 [details]
corrected patch

berend. I overworked the patch
(Assignee)

Comment 15

12 years ago
Created attachment 258304 [details]
corrected patch

berend. I overworked the patch
Attachment #258304 - Flags: first-review?(michael.buettner)
(Assignee)

Comment 16

12 years ago
Created attachment 258305 [details] [diff] [review]
corrected patch

berend. I overworked the patch
Attachment #258305 - Flags: first-review?(michael.buettner)
Comment on attachment 258304 [details]
corrected patch

I think this patch has been submitted twice, marking obsolete.
Attachment #258304 - Attachment is obsolete: true
Attachment #258304 - Flags: first-review?(michael.buettner)
Attachment #258303 - Attachment is obsolete: true
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-
Attachment #258305 - Flags: first-review?(michael.buettner) → first-review-
(Assignee)

Comment 19

12 years ago
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
(Assignee)

Comment 20

12 years ago
Created attachment 263252 [details] [diff] [review]
preliminary patch
Attachment #263252 - Attachment is patch: true
Attachment #263252 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 21

12 years ago
Created attachment 264254 [details] [diff] [review]
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.
Attachment #258305 - Attachment is obsolete: true
Attachment #263252 - Attachment is obsolete: true
Attachment #264254 - Flags: review?(michael.buettner)
(Assignee)

Updated

12 years ago
Attachment #264254 - Attachment is patch: true
Attachment #264254 - Attachment mime type: text/x-patch → text/plain
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.
Attachment #264254 - Flags: review?(michael.buettner) → review-
(Assignee)

Comment 23

12 years ago
Created attachment 265653 [details] [diff] [review]
The very new patch

I fixed the remaining bug about the invalid enddate when its an all-day event.
Attachment #258301 - Attachment is obsolete: true
Attachment #264254 - Attachment is obsolete: true
Attachment #265653 - Flags: review?(michael.buettner)
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.
Attachment #265653 - Flags: review?(michael.buettner) → review-
(Assignee)

Comment 25

12 years ago
Created attachment 265673 [details] [diff] [review]
The patch that resolves the remaining issue

I resolved the remaining issue.
Attachment #265653 - Attachment is obsolete: true
Attachment #265673 - Flags: review?(michael.buettner)
(Assignee)

Comment 26

12 years ago
Created attachment 265796 [details] [diff] [review]
a new patch...

Found another issue that I fixed within this patch
Attachment #265673 - Attachment is obsolete: true
Attachment #265796 - Flags: review?(michael.buettner)
Attachment #265673 - Flags: review?(michael.buettner)
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()).
Attachment #265796 - Flags: review?(michael.buettner) → review-
(Assignee)

Comment 28

12 years ago
(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.
(Assignee)

Comment 29

12 years ago
(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.
(Assignee)

Comment 30

12 years ago
Created attachment 266905 [details] [diff] [review]
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.
Attachment #265796 - Attachment is obsolete: true
Attachment #266905 - Flags: review?(michael.buettner)
Comment on attachment 266905 [details] [diff] [review]
final patch

Everything looks fine, thanks for the patch. r=mickey.
Attachment #266905 - Attachment description: the newes patch → final patch
Attachment #266905 - Flags: review?(michael.buettner) → review+
Whiteboard: [checkin needed after 0.5]
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Hardware: PC → All
Whiteboard: [checkin needed after 0.5]
Target Milestone: --- → 0.7
(Reporter)

Comment 33

12 years ago
Verified with Lightning 2007070403 and lightning-wcap (20070704)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.