Community
Participate
Working Groups
This is a follow up from bug#195691 about the new requirements emerged from screenshot attachment story. While working on that bug, I felt the need to add more features to LocalAttachment class. Since it is actually strongly tied to the notion of content coming from either a file or a byte[] from clipboard, and due to the time constraints to get it into 2.3, I opted to do the simplest thing that could possibly work, and subclassed it, adding my needs into ImageAttachment class. Now it is time to refactor :) The screenshot attachment story has some specific needs: - The attachment is created from a business object (Image), not a existing file. - The object being attached is not the same that is being created from. The Image needs to be converted into JPEG data, which is what is attached. So, there is a notion of attachment source, which produces the final attachment data. - The attachment data production (convert into JPEG or any image format) is an expensive operation. It must be done lazily. The attachment object must have a method to trigger this data production. - The data production must be triggered only if needed. Actually, it is needed not only when you hit finish (obviously), but when you go to the preview page (to display the exactly image which will be submitted). If you go to preview and hit finish, you don't need to create the data again. - The user may go to preview page (triggering the data production), go back to crop/mark up page, and modify the original image. In this case, the final data must be recreated when you return to preview page or if you hit finish directly. The attachment model must support the notion of "dirty" data. - The attachment model should have a "clean" callback method, so the temporary image file may be deleted after submission. Now my opinion about LocalAttachment class: - It's object interface is strongly tied to the existing attachment contents/sources. It is either a byte[] for clipboard attachment, or a File. This doesn't smell good, and it should be good to decouple and/or generalize content handling, and only leave general attachment attribute handling on this class.
I forgot to say: I guess a good initial approach would be to merge LocalAttachment and ImageAttachment, decoupling the content source (clipboard/file/image) into a delegating interface.
That sounds like a great proposal. Separating attachment meta data from the content will significantly improve the attachment API and make it more extensible. I would like to consider additional requirements in this refactoring of the attachment data model for 3.0: - Offline support for attachments: Provide a facility in the tasks framework for caching downloaded attachments. Also support saving of attachments to submit them at a later time. A client that is accessing attachments should not be concerned how the content of an attachment is handled, i.e. if it is stored in a cached local file or in the remote repository or if it has not been submitted, yet. - Unify ITaskAttachment, FileAttachment, RepositoryAttachment, LocalAttachment and ImageAttachment into a single class hierarchy that is decoupled from TaskData. - Decouple the attachment wizard from the task editor. This refactoring ties into other changes I have been making to the way task data and attributes are handled. To get started I would like to create a new class TaskAttachment that has a reference to a source object that provides the actual content, e.g.: public abstract class AbstractTaskAttachmentSource { /** Returns the content of the attachment. */ public abstract InputStream createInputStream(IProgressMonitor monitor) throws CoreException; /** The default file name for the attachment. */ public abstract String getName(); /** The size of the attachment. */ public abstract long getLength(); /** Disposes any temporary resources. */ public void dispose(); } In the case of attaching screenshots the attachment source would be managed by the attachment wizard or screenshot page. It could create a temporary file on the first call to createInputStream() that could be deleted once the source is disposed. It would be recreated in case the screenshot is disposed after it was created. My thinking is to duplicate the current attachment wizard implementation (i.e. create a NewAttachmentWizard2 class) and to start hacking on it without risking to break the current implementation. I could take a first pass on the weekend starting with the data model.
Created attachment 92646 [details] patch
I have attached the first pass for refactoring the data model. The code in the patch is incomplete and does not work. It is just meant as a basis for further discussion. Willian, it would be great if you could take a look at AbstractTaskAttachmentSource and its implementations in TaskAttachmentWizard. AbstractTaskAttachmentSource is the class that I am proposing for separating attachment data from attachment properties. Here is how this could work: - The first wizard page creates a source object that is an instance of AbstractTaskAttachmentSource. In the case of the screenshot page the source object could be managed in the attachment wizard to keep it separated from attachment specific code. Alternatively a generic screenshot editor class could be extracted that is not coupled to a wizard page and would be even more reusable by other projects. - The second page is the preview page that displays the contents of the source object by invoking reateInputStream() on the source. The ImageSource class used for screenshots could (re)generate the image in this method if needed. - The third page is a potentially connector specific page for editing properties of the TaskAttachment object. If this make sense to you I will go ahead and implement the data model specific parts to make the patch actually work. Based on that it would be great if you could do the refactoring of the attachment wizard / image generation / preview page code. What do you think?
Created attachment 92648 [details] mylyn/context/zip
Hi Steffen, I think it is a good start. Some questions: 1 - Why did you move the dirty state handling into ScreenshotAttachmentPage? I'm not sure if it is a good idea to have this state into a view. Actually, this attribute is only need by the screenshot/image attachment type, but it sounds like the best place to it is in the attachment model, because it is tied to the attachment data lifecycle. 2 - This is related to (1): do you think it is better to move laziness handling out of the attachment model? The problem I see is that clients of model will have to track if the data was created or not, or it must be recreated. Also, with (1), you have the preview page and the attachment creation job interacting with the screenshot editor page to handle the dirtiness. This is something that worried me when working on ImageAttachment, and I thought an better design was to move this state into the attachment model. In ImageAttachment, they are markDirty() and ensureImageFileWasCreated() methods. What about moving markDirty() into TaskAttachment, and renaming/moving ensureImageFileWasCreated() to some more generic name, like prepareAttachmentData() (or a better name)? prepareAttachmentData() is a no-op for attachments that don't need sophisticated data handling (anything but screenshot attachment, actually). It could be an empty implementation in TaskAttachment; overrided and implemented in ScreenshotAttachment. prepareAttachmentData() is called from every place that needs the "final" data: actually preview page and attachment job. This method is lazy: the second call has no effect, and the clients of this class don't need to know the dirty state; just call the method and let it handle all checks for you. markDirty() should be only used by ScreenshotAttachment and other possible attachment types that modify the object being attached. Other attachment types shouldn't have to ever need to know about this method; just ignore it.
Created attachment 92668 [details] Patch I think it is better to explain with code. This patch contains proposal for modifications in TaskAttachmentWizard and AbstractTaskAttachmentSource reflecting my previous comment. In TaskAttachmentWizard I only modified the ImageSource inner class to fit the proposal. If we go on with this design, the ensureAttachmentDataWasCreated() calls in preview page and attachment job should be modified to something like getAttachment().getAttachmentSource().prepareAttachmentData(). The markDirty() in ScreenshotAttachmentPage should be modified to getAttachment().getAttachmentSource().markDirty(). This is an quick and dirty code, I didn't really tested it. * I added dispose() method that was missing in the original AbstractTaskAttachmentSource.
Created attachment 92669 [details] mylyn/context/zip
I designed the AbstractTaskAttachmentSource class with IStorageEditorInput in mind. IStorageEditorInput provides an abstract way to access content by providing a lightweight object that does not hold on the actual data model but is closer to a descriptor. In the case of ScreenshotAttachmentPage I think the data model is the image that is managed in the page. My sense is that the dirty state is a property of the page or of that image but not of the data input. The ImageSource has an optimization that stores the image (data model) in a temporary file but it could as well always recreate the image and not worry about writing to a temporary file or managing dirty state. Putting the handling of the dirty state into the abstract class makes the API contract of AbstractTaskAttachmentSource more complicated although this functionality is only needed for screenshots. I am not sure what the benefit is of exposing the optimization that is only used by ImageSource to every client (preview page and attachment job) and implementor (FileSource, ClipboardSource etc.)?
I have committed a first pass of the revised API to head. The implementation is along the proposal in comment 2: An object of type TaskAttachmentModel is shared between the wizard pages. The model has a reference to the attachment source which is determined by the selection on the first wizard page. The source provides an input stream to access it's data. The input stream is used for the preview and attachment submission. The meta data that is provided by the user is stored on a TaskAttribute and managed by the connector. A new factory method has been added to AbstractRepositoryConnectorUi that allows connectors to provide their own page for editing, e.g. JIRA needs to disable the description field: public IWizardPage getAttachmentPage(TaskAttachmentModel model); The default implementation will use the attribute mapping that is specified in TaskAttachment.
Created attachment 98295 [details] mylyn/context/zip
