Opened 7 years ago

Closed 7 years ago

#388 closed enhancement (fixed)

add beforedownload event in PrintProvider

Reported by: bartvde Owned by:
Priority: major Milestone: 1.1
Component: GeoExt.data.PrintProvider Version: 1.0
Keywords: Cc:
State: Commit

Description

Currently the PrintProvider opens up a new window in Internet Explorer. We are having some issues with IE8 settings and people not getting access to the opened up tab, so we want to change the flow a bit by opening up a new window with an HTML page that contains a link to the PDF, so that the download is triggered by explicit user interaction.

In order to do so, we suggest introducing a beforedownload event in PrintProvider. By returning false, we can cancel the default processing and hook in our own.

Attachments (3)

geoext-388.patch (2.5 KB) - added by bartvde 7 years ago.
geoext-388.2.patch (3.1 KB) - added by ahocevar 7 years ago.
geoext-388.3.patch (4.5 KB) - added by bartvde 7 years ago.
now with testcase

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by bartvde

comment:1 Changed 7 years ago by bartvde

  • State changed from None to Review

PrintProvider tests still pass in IE8 and FF3.6, please review.

comment:2 Changed 7 years ago by ahocevar

  • State changed from Review to Needs more work

I think the beforedownload event should also be triggered for GET requests, and the default way of opening the pdf way should be the same regardless of the request method. See my patch - at least one unit test would be in order though.

Changed 7 years ago by ahocevar

comment:3 Changed 7 years ago by bartvde

Hi Andreas, your change makes sense, I'll look into adding some tests on Monday.

Changed 7 years ago by bartvde

now with testcase

comment:4 Changed 7 years ago by bartvde

  • State changed from Needs more work to Review

Andreas, I've added a testcase. Is this what you had in mind?

Btw, one more question, in my patch I did not fire the "print" event if beforedownload would return false, since the event description reads "Triggered when the print document is opened.", and the print document will not be opened in that case. In your patch, you changed this. Do you still think this is valid?

comment:5 Changed 7 years ago by ahocevar

  • State changed from Review to Commit

@bartvde, the problem with the print event is: if you don't abort by a listener returning false from the beforeprint event, you would expect the print event to be fired, right? If there was a download event, I'd agree that it shouldn't be fired when a listener returns false from the beforedownload event. If we still were in pre-1.0, I'd say let's rename the beforeprint event to beforeencode, and the beforedownload event to beforeprint. Then your proposal would make sense, and things would be more consistent overall. Maybe you want to add a TODO for this.

Having said that, patch and tests look good. Please commit.

comment:6 Changed 7 years ago by bartvde

  • Resolution set to fixed
  • Status changed from new to closed

(In [2526]) add beforedownload event in PrintProvider, r=ahocevar (closes #388)

Note: See TracTickets for help on using tickets.