Community
Participate
Working Groups
I would like to see being able to mark an attachment with the iplog+ flag. I do a lot of reviewing and committing of contributions, and it would be nice to not have to switch to the web browser view every time for that.
Rob, I start to create an Dialoag that show all Bugzilla Attributes and allow to change the same attributes that are changeable in the http page. Is this OK?
That sounds great to me. At some point we can consider a less intrusive workflow and inline the attachment attributed in the editor form but I think a dialog is a good solution for now.
Yes, sounds good Frank. Post a screenshot before going to deap on ui.
Created attachment 133266 [details] screenshot of the dialog Here is a first screenshot. The Dialog extends SelectionDialog and use the AttributeEditorFactory for drawing. This Dialog is used for showing the Details and for Update. All background work is finished.
Hi Frank, Your dialog looks very nice! A few spelling errors: "obsolate" should be "obsolete" "Attachmet" should be "Attachment" "Size in Byte" should be "Size in Bytes" For the content type, should that be a drop down list with common MIME types instead of a simple text field? Should the filename field have a button on the right side to open a file prompter to select a different file? Or is that a read-only field based on a previous selection? I assume there will be text in the dialog's title bar. David
Yes, looking good Frank. All good points David. Lets stick with a text field for content type or editable combo perhaps. I think we can eliminate the Token, Author, and creation date from this dialog. Consider changing flags to 'Advanced' and placing the flags and url fields within. Would an order like this work: * Description: * Filename: * Size in Bytes: * [x] patch [x] obsolete + Advanced * Flag * Url * New Comment Frank, do you see this being invoked from a Details... action within the attachment popup?
Created attachment 135233 [details] first implementation Thoughts?
Created attachment 135234 [details] mylyn/context/zip
Frank, Rob, should this patch be considered for inclusion in Mylyn 3.2?
(In reply to comment #9) > Frank, Rob, should this patch be considered for inclusion in Mylyn 3.2? > I wait for feedback and hope that we are able to include this in 3.2.
I have noticed that if an attachment is marked obsolete, in the Mylyn bugzilla task editor, it does not appear differently from a valid attachment. In the web browser, the attachment appears with strikethru text. Is there any kind of visual indicator we can use to indicate obsolete in the bugzilla task editor? I don't know if you can do strikethrough text in this table control, so if not, could you use grayed out text or something? I am assuming this should go into a separate defect, correct?
Obsolete attachments should be shown with a grey font color. Which version of Mylyn are you using?
(In reply to comment #12) > Obsolete attachments should be shown with a grey font color. Which version of > Mylyn are you using? Sorry, I'm a bit backlevel. I'm on the version compatible with Eclipse 3.4 GA.
I have taken a first pass at reviewing the patch. The approach looks right and it's great how it reuses the attribute editor framework. Before applying the patch I would like to discuss two things: The proposed change to TaskDataModel would cause child attributes of a task attachment attribute to get stored in the list of edited attributes which is intended to only store top-level nodes. This could have bad side-effects if a top-level node had the same id. I think we should copy the hierarchy of parent attributes instead but I this is a non-trivial change. I would also like to decouple the attachment dialog from the editor page. The data model should either be available through a shared manager or the dialog should use it's own data model. I am not sure what the best approach is here and want to take some more time to look at other platform implementations. Since we are already behind schedule for the 3.2 release, is it okay if we consider the patch early in the next release cycle?
Created attachment 137420 [details] next try (In reply to comment #14) > I would also like to decouple the attachment dialog from the editor page. The > data model should either be available through a shared manager or the dialog > should use it's own data model. This was almost there. The model now only include the childs from the attachment and no longer the whole attachment. > I am not sure what the best approach is here and > want to take some more time to look at other platform implementations. > > Since we are already behind schedule for the 3.2 release, is it okay if we > consider the patch early in the next release cycle? For me it is OK to do this in 3.3
Created attachment 137421 [details] mylyn/context/zip
Is this for 3.3 or for 3.2.1?
All enhancements should go into the 3.3 stream. The 3.2.1 is reserved for trivial stream-lining or important bugfixes only.
Created attachment 145341 [details] patch for 3.3 I recreate the patch, Rob please review.
Created attachment 145342 [details] mylyn/context/zip
Reviewing...
Created attachment 148590 [details] minor updates Looks excellent Frank. Just have some minor nits I'm going to review here. Will post a follow up to this later today. This update just includes minor changes to messages and removed some debug output of stack traces. You will want to look at BugzillaAttachmentUpdateAction exception handling upon save. I replace the stack trace dump with a logging statement but may need to result in failure? Other nits I'll look at this afternoon: * Dialog title likely doesn't need attachment id * Need to investigate how the ui appears on other platforms for the details/update dialogs. On Mac some transparency is incorrect but I believe this is a platform bug. * Non-editable text fields (i.e. url) should likely be selectable
Created attachment 148596 [details] ui nits fixed * Border added to comment * Issue with BugzillaAttachmentUpdateAction creating bogus task upon opening details dialog * Dialog titles simplified * Transparency fixes * Border added to comment * Issue with BugzillaAttachmentUpdateAction creating bogus task upon opening details dialog * Dialog titles simplified I think before we proceed further, we need to resolve the problem of BugzillaAttachmentUpdateAction creating a bogus 'attachment' task in the task list upon opening the details dialog.
Moving to 3.4 pending resolution of the task creation issue mentioned in previous comment.
(In reply to comment #24) > Moving to 3.4 pending resolution of the task creation issue mentioned in > previous comment. Rob, sorry I can not reproduce this. Do you have more details on this?
Created attachment 155778 [details] screenshot
Rob, what is the status of this bug?
Frank, creation of the working copy in BugzillaAttachmentUpdateAction.run(IAction action) results in an invalid task being added to the task list (look under Uncategorized for tasks with a name like: 272207attachment:). If the working copy is constructed in order to use attribute field editors in the dialog perhaps we should consider using regular widgets (or maybe the model save can just not be done so as not to create a task in the task list). I'm not able to change the iplog status with this patch as applied to head. It appears that the post has the wrong key names: task.common.kind.flag2= I believe this needs to be flag-2 to result in the update. Please include a unit test for updating attachment flags.
Created attachment 156125 [details] minor update Minor update to apply to head.
Created attachment 156335 [details] patch to commit Rob, please review before I commit this to HEAD.
Created attachment 156336 [details] mylyn/context/zip
(In reply to comment #30) > Created an attachment (id=156335) > patch to commit > > Rob, > > please review before I commit this to HEAD. Thanks Frank, I'll look at this Thursday.
Great. I updated the dialog layout/indentation. Might be better if the ispatch and obsolete were side by side but left aligned. Can you look into this? I also enabled the URL fields so that people can click on it. In the popup menu, can we just hae a single "Details..." action that shows the current "Update" version of the dialog? They appear to show the same information just one is read only while the other you can submit changes. Is there a reason for two? One functional nit is that when I tested modifying a flag and entered a user id in the requestee field, it did not make it to the server. The flag state change did work however. So once this functional nit is address and we sort out if we can merge the two versions of the dialog into one, I think you'll be good to apply the patch. I'll post an updated patch for you with my changes...
Created attachment 156860 [details] ui updates
Created attachment 157056 [details] commited patch Here is what I have commit to HEAD.
Created attachment 157057 [details] mylyn/context/zip
(In reply to comment #33) > Great. I updated the dialog layout/indentation. Might be better if the ispatch > and obsolete were side by side but left aligned. Can you look into this? I also > enabled the URL fields so that people can click on it. > > In the popup menu, can we just hae a single "Details..." action that shows the > current "Update" version of the dialog? They appear to show the same information > just one is read only while the other you can submit changes. Is there a reason > for two? Now we only have on action in the popup. I change the OK Button to Update and disabled. Only when you change a field then this button get enabled. > > One functional nit is that when I tested modifying a flag and entered a user id > in the requestee field, it did not make it to the server. The flag state change > did work however. When you change that field and then hit the OK button the focusLost did not get triggered. Now this is fixed. > > So once this functional nit is address and we sort out if we can merge the two > versions of the dialog into one, I think you'll be good to apply the patch. > I'll post an updated patch for you with my changes...
Created attachment 157179 [details] screenshot
Great stuff! I have added an item to next weeks meeting agenda to do a UI review.
Created attachment 157488 [details] UI with scrollable Flagsection I ad a ScrolledComposit so you can have more flags witout the need big space to show them all.
(In reply to comment #40) > Created an attachment (id=157488) > UI with scrollable Flagsection > > I ad a ScrolledComposit so you can have more flags witout the need big space to > show them all. The review took place on the all today Frank. The ui is looking great overall with only a few suggestions raised to improve consistency and usability. Consider creating new subtasks to address these: * To keep buttons and banners consistent, use a wizard / wizard page. Not only will this make the buttons consistent but will also add the currently missing banner. Additionally you'll be able to show progress right there within the wizard page during the post. * The consensus regarding the scrolled composite was that the interaction/usability may be difficult given the dialog dimensions. The recommendation is to update the dialog so that the whole composite scrolls (scrolling pane). An example from platform is the preference page: Java > Compiler > Errors/Warnings
I noticed that the advanced section shows even if there are now flags (e.g. when adding an attachment to this bug). Can we hide it in this case?
As discussed on today's call I have reverted the patch in the e_3_6_m_3_3_x branch so it won't get included in the maintenance release. It is still in head and will be released with Mylyn 3.4.
Created attachment 157719 [details] screenshot Here is a screenshot of the actual implementation. Should we do a review or can a commit my changes?
Created attachment 157837 [details] screenshot Dialog with scroller
Looks good. Can we remove the border around the scrolled pane though and does the scrollbar disappear if there is sufficient screen estate?
(In reply to comment #46) > Looks good. Can we remove the border around the scrolled pane though and does > the scrollbar disappear if there is sufficient screen estate? I removed the SWT.BORDER from the style. For the scrollbar look in attachment (id=157719) and you see that there is no scrollbar.
Created attachment 158090 [details] patch the current sources to save until review is done.
Created attachment 158091 [details] mylyn/context/zip
fyi, BugzillaAttachmentWizardPage didn't make it into that patch.
Again, another ui review on today's call Frank. Its looking good. Here are the the nits raised on the call: * Although not unanimous, it seems as though removal of the border is getting a +1 * Suggestion was raised to change the indenting of the advanced section to be similar to the task repository setting wizard ** Alignment of section headers should be directly below labels above (currently a its a few pixels to the right I believe) ** body content tabbed over slightly * I had forgot to mention it before but the labels appear to be using form colors and should be using the dialog font colors. You may need to coordinate with Steffen on this if it requires changes to attribute editors. * A great idea was raised to move the comment out of the scrollable to the bottom of the dialog - it is different from all the other attributes and is comment about the changes made to these attributes. * If possible, also try to move the patch checkbox to the same line (and right of) the content type field
Created attachment 158395 [details] commited patch (In reply to comment #51) > Again, another ui review on today's call Frank. Its looking good. Here are the > the nits raised on the call: All done. Leave this bug open, so you can add comments. Btw You can change the size of the comment when you find the line between the two controls.
Created attachment 158396 [details] mylyn/context/zip
Created attachment 158871 [details] screenshot
Looking great Frank! I'll add to today's agenda.
+ 1 all around Frank. Only suggestion was to reduce the height of the comment text box (possibly even 1/2 its current height). Other than that I think we're done here.
(In reply to comment #56) > + 1 all around Frank. Only suggestion was to reduce the height of the comment > text box (possibly even 1/2 its current height). Other than that I think we're > done here. I changed the Weights from { 65, 35 } to { 75, 25 } Pleas have a look. If you think that I should change the value please reopen this bug.
Got this exception while trying to open details for attachment on bug 262368: java.lang.IllegalArgumentException: Unsupported editor type: "long" at org.eclipse.mylyn.tasks.ui.editors.AttributeEditorFactory.createEditor(AttributeEditorFactory.java:142) at org.eclipse.mylyn.internal.bugzilla.ui.action.BugzillaAttachmentUpdateAction$1.createEditor(BugzillaAttachmentUpdateAction.java:120) at org.eclipse.mylyn.internal.bugzilla.ui.wizard.BugzillaAttachmentWizardPage.createAttributeEditor(BugzillaAttachmentWizardPage.java:173) at org.eclipse.mylyn.internal.bugzilla.ui.wizard.BugzillaAttachmentWizardPage.createAttributeEditors(BugzillaAttachmentWizardPage.java:224) at org.eclipse.mylyn.internal.bugzilla.ui.wizard.BugzillaAttachmentWizardPage.createControl(BugzillaAttachmentWizardPage.java:148) at org.eclipse.jface.wizard.Wizard.createPageControls(Wizard.java:170) at org.eclipse.jface.wizard.WizardDialog.createPageControls(WizardDialog.java:675) at org.eclipse.jface.wizard.WizardDialog.createContents(WizardDialog.java:549) at org.eclipse.jface.window.Window.create(Window.java:431) at org.eclipse.jface.dialogs.Dialog.create(Dialog.java:1089) at org.eclipse.mylyn.internal.bugzilla.ui.action.BugzillaAttachmentUpdateAction.run(BugzillaAttachmentUpdateAction.java:145) ...
The exception in comment 58 can easily be fixed by synchronizing the task.
Created attachment 161227 [details] screenshot of attachment detail on Win XP (In reply to comment #59) > The exception in comment 58 can easily be fixed by synchronizing the task. Isn't it something that is worth to fix? I also got this exception and thought the feature is not working until stumbled into this workaround. The first time I opened the dialog it opened wide and when I expanded "Advanced" the window collapsed to the smaller size unexpectedly. The second time I opened it it was the smaller size (which is OK). There are a couple of things not quite neat (I am on Win XP): - One is that there is an unindentified box next to iplog. It is probably something useful but there is no clue. I cannot enter anything in it either. - Another nitpick is that after clicking "Advanced" it shifts just a few pixels up which does not look quite nice. Is it possible to ajust layout just a little bit? Looking at Mylyn version 3.4.0.I20100305-1500-e3x.
