Vaadin 8 compatibility #27

WoozyG · 2017-03-03T18:22:48Z

You will want to review the POM changes - I think I only modified version #s (made it v8), and dependencies.

I removed the Viritin dependency, as it is a large library with multiple dependencies, and used only for string escaping, which is done differently/better now anyway in Vaadin. Added Commons-IO explicitly (was a transitive dependency from Viritin) - it is only used for FileUtils.byteCountToDisplaySize(long).

I think the UI test server framework you referenced in your comment needs updating for Vaadin 8 - I couldn't get the Jetty instance to launch, complaining about a method not found. I think the argument type changed packages in V8 or something.

This works for me in my project, drag/drop, clicking and selecting a file/files, displaying in-process uploads (using the newer progress component) etc.

fix bounds check for maxFileCount to allow for uploading 1 file

Removed Viritin dependency - was only for one tiny piece, and pulled in a large amount of library code. Replaced with Commons-IO instead, which is a dependency for Vaadin-charts, among other things, and much smaller. Only used for byteCountToDisplaySize() method.

mstahv · 2017-03-08T13:28:10Z

Looks good! I have a lot of things to do this week, I hope somebody else familiar with the codebase could look through the changes so we could get the first release out soon. @Ansku, @wbadam?

wbadam

Hi Greg,

Thanks for contributing, it looks good. I recommended are a few small changes and questions, please take a look when you have time.

@mstahv if you could look at my comments as well, that would be good.

Thanks,
Adam

wbadam · 2017-03-10T14:47:52Z

pom.xml

@@ -142,6 +142,7 @@
                            <goal>compile</goal>
                        </goals>
                        <configuration>
+							<extraJvmArgs>-Xmx1G</extraJvmArgs>


Do you need this parameter? I'm not having out of memory without it.

Yes, at least for me with Maven + Eclipse + V8 I needed it, and found it documented in the V8 migration guide:

https://vaadin.com/vaadin-fw8-documentation-portlet/framework/migration/migrating-to-vaadin8.html

Perhaps my Eclipse setup uses a different, lower default configuration than however you ran it?

wbadam · 2017-03-10T14:50:02Z

pom.xml

@@ -294,6 +295,30 @@
        </dependency>
        <dependency>
            <groupId>com.vaadin</groupId>
+            <artifactId>vaadin-compatibility-server</artifactId>


Please remove vaadin-compatibility-client and vaadin-compatibility-server packages, they are not in use

Good catch. Found one unused import still referencing them, but otherwise I'd updated uses but left the dependencies. Will remove them in a new pull request.

wbadam · 2017-03-10T14:52:27Z

src/main/java/org/vaadin/easyuploads/Widgetset.gwt.xml

@@ -4,7 +4,8 @@
 	<!-- WS Compiler: manually edited -->

 	<inherits name="org.swfupload.SWFUpload" />
-	<inherits name="com.vaadin.terminal.gwt.DefaultWidgetSet" />
+    <inherits name="com.vaadin.DefaultWidgetSet" />
+    <inherits name="com.vaadin.v7.Vaadin7WidgetSet" />


Please remove the Vaadin7WidgetSet since there are no compatibility components in use

wbadam · 2017-03-10T15:21:13Z

src/main/java/org/vaadin/easyuploads/client/ui/VMultiUpload.java

@@ -292,7 +296,7 @@ private void submit() {
            } else if (!AcceptUtil.accepted(file.getName(), file.getType(),
                    accepted)) {
                noMatches.add(file);
-            } else if (maxFileCount != null && filedetails.size() >= maxFileCount) {
+            } else if (maxFileCount != null && filedetails.size() > maxFileCount) {


I think this should stay >= because when maxFileCount is set to 1, on the second iteration filedetails.size() will still be 1 at this point of code and the file should be added to tooMany, so condition should be true. That will happen when = is also accepted.

Without this change I can't click to open the browser file selection dialog and successfully choose only one file. I filed this as bug # 25 a while back. Without this you get a message like "0 files selected, but the maximum allowed is 1."

wbadam · 2017-03-10T18:19:35Z

src/main/java/org/vaadin/easyuploads/UploadField.java

@@ -79,7 +79,7 @@
    private Upload upload;
    protected Component display;

-    private ProgressIndicator progress = new ProgressIndicator();
+    private ProgressBar progress = new ProgressBar();


@mstahv we need to mention in the documentation that setting getUI().setPollInterval() may be needed or need to support it internally somehow.
Currently the progress bar is not updated by default.

wbadam · 2017-03-10T18:48:02Z

src/main/java/org/vaadin/easyuploads/UploadField.java

@@ -573,41 +534,29 @@ public void removeListener(Property.ValueChangeListener listener) {
     * validated before the event is created.
     */
    protected void fireValueChange(boolean repaintIsNotNeeded) {
-        fireEvent(new AbstractField.ValueChangeEvent(this));
+        fireEvent(new HasValue.ValueChangeEvent(this, this, null, ! repaintIsNotNeeded));


It may not be important, but could perhaps pass the old value, at least when it's known as in setValue() method. Not a blocker I guess.

wbadam · 2017-03-10T18:51:24Z

src/main/java/org/vaadin/easyuploads/UploadField.java

     * Tests the current value against all registered validators.
     * 
     * @return <code>true</code> if all registered validators claim that the
     *         current value is valid, <code>false</code> otherwise.
     */
-    public boolean isValid() {
+	public boolean isValid() {


Does this method make sense without the validation?

Maybe not. Is is problematic to leave it?

mstahv · 2017-03-16T15:37:19Z

I tried the changes in this pull request. I also had to change some dependencies to properly run the test UIs. There was still quite a lot of non-functional stuff there.

You changes and my additions are now in https://github.com/parttio/easyuploads/tree/WoozyG-master

Some issues are quite hard to resolve, like typing in fields. MultiFileUpload is probably easier to tackle as it is not a Field. It might be best to have a fresh start with V8 and its HasFields interface and leave old stuff to compatibility package. I think I'll do a "quick-and-dirty" V8 upgrade first and then we can look into V8 native UploadField etc.

WoozyG · 2017-03-16T15:40:13Z

Thanks for checking it. I only use a small slice of the functionality myself, and it works for that purposes, but since I couldn't get the UI to run for manual testing I couldn't see the rest. I agree, it may be best to not try to make some parts V8 native. Possibly time to drop the Flash uploader entirely?

…

On Thu, Mar 16, 2017 at 8:37 AM Matti Tahvonen ***@***.***> wrote: I tried the changes in this pull request. I also had to change some dependencies to properly run the test UIs. There was still quite a lot of non-functional stuff there. You changes and my additions are now in https://github.com/parttio/easyuploads/tree/WoozyG-master Some issues are quite hard to resolve, like typing in fields. MultiFileUpload is probably easier to tackle as it is not a Field. It might be best to have a fresh start with V8 and its HasFields interface and leave old stuff to compatibility package. I think I'll do a "quick-and-dirty" V8 upgrade first and then we can look into V8 native UploadField etc. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4IfbctCvQxoYOdREhDxyBM5qzV5npdks5rmVcvgaJpZM4MSlZb> .

mstahv · 2017-03-16T15:42:23Z

Yep, IIRC the flash uploader thingie is no more needed with IE11 (or any other modern browser). Hitting delete on that will bring a big smile on my face soon.

mstahv · 2017-03-17T12:09:20Z

@WoozyG Can you try a build from the latest master with your application and let me know if it works.

I couldn't yet make all tests pass with the master either, but that appeared to be a Vaadin core issue, which should be fixed soon in 8.0.3.

Could you also provide a test case for #25 ? I could then look into what is actually happening there, the proposed fix look somehow wrong.

mstahv · 2017-04-21T02:03:27Z

This was implemented in a separate changeset.

WoozyG added 3 commits February 28, 2017 18:05

Updated for Vaadin 8

0ca416e

https://github.com/parttio/easyuploads/issues/25

d0769ee

fix bounds check for maxFileCount to allow for uploading 1 file

wbadam requested changes Mar 10, 2017

View reviewed changes

Remove V7 compatibility dependencies, as they aren't used.

ef58295

mstahv closed this Apr 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vaadin 8 compatibility #27

Vaadin 8 compatibility #27

WoozyG commented Mar 3, 2017

mstahv commented Mar 8, 2017

wbadam left a comment

wbadam Mar 10, 2017

WoozyG Mar 10, 2017

wbadam Mar 10, 2017

WoozyG Mar 10, 2017

wbadam Mar 10, 2017

WoozyG Mar 10, 2017

wbadam Mar 10, 2017

WoozyG Mar 10, 2017

wbadam Mar 10, 2017

wbadam Mar 10, 2017

wbadam Mar 10, 2017

WoozyG Mar 10, 2017

mstahv commented Mar 16, 2017

WoozyG commented Mar 16, 2017 via email

mstahv commented Mar 16, 2017

mstahv commented Mar 17, 2017

mstahv commented Apr 21, 2017

Vaadin 8 compatibility #27

Vaadin 8 compatibility #27

Conversation

WoozyG commented Mar 3, 2017

mstahv commented Mar 8, 2017

wbadam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstahv commented Mar 16, 2017

WoozyG commented Mar 16, 2017 via email

mstahv commented Mar 16, 2017

mstahv commented Mar 17, 2017

mstahv commented Apr 21, 2017