jeandersonbc / gsoc14-planning

Planning for accepted project on Google Summer of Code 14
1 stars 0 forks source link

Identify outdated JFace snippets and delete them #5

Closed vogella closed 10 years ago

vogella commented 10 years ago

If you see outdated JFace snippets, e.g. workaround which have been solved later, I suggest to delete them.

jeandersonbc commented 10 years ago

Hello @vogella,

I just saw your comment after updating the commit message here (https://git.eclipse.org/r/#/c/25762/). I'm going to investigate this. Should we just file a bug report on bugzilla for this task?

vogella commented 10 years ago

I think we can handle that via the Clean-up bug in Bugzilla. Deleting "old" stuff is also a cleanup activity.

2014-04-30 7:47 GMT+02:00 Jeanderson Barros Candido < notifications@github.com>:

Hello @vogella https://github.com/vogella,

I just saw your comment after updating the commit message here ( https://git.eclipse.org/r/#/c/25762/). I'm going to investigate this. Should we just file a bug report on bugzilla for this task?

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-41762862 .

jeandersonbc commented 10 years ago

@vogella

Could you give me some directions on how to identify outdated snippets? I was browsing https://wiki.eclipse.org/JFaceSnippets to see if there was any useful information but I'm a little confused and the documentation doesn't give much information about it. More precisely, what I should look for to judge what is outdated or not?

Because we are talking about file deletion, I think we should identify and delete those snippets first before continue the cleanup.

If @hendrikstill has any information about it too, it would be very helpful.

Thanks in advance

hendrikstill commented 10 years ago

Because we are talking about file deletion, I think we should identify and delete those snippets first before continue the cleanup.

If @hendrikstill has any information about it too, it would be very helpful.

I guess outdated snippets, would be snippets which demonstrate the usage of deprecated api or snippets which demonstrate the "old style" use of the api. For the 031 Snippet the JavaDoc comment about the class says:

This code is for users pre 3.3 others could use newly added tooltip support in {@link CellLabelProvider}

@vogella are you sure that also snippets like this should be removed?

vogella commented 10 years ago

@vogella https://github.com/vogella are you sure that also snippets like this should be removed?

Yes. I see no added value in describing an outdated API via a snippet. If someone requires that, he can look it up via Git. And 3.2 is really, really old.

2014-05-03 11:48 GMT+02:00 Hendrik S notifications@github.com:

Because we are talking about file deletion, I think we should identify and delete those snippets first before continue the cleanup.

If @hendrikstill https://github.com/hendrikstill has any information about it too, it would be very helpful.

I guess outdated snippets, would be snippets which demonstrate the usage of deprecated api or snippets which demonstrate the "old style" use of the api. For the 031 Snippet the JavaDoc comment about the class says:

This code is for users pre 3.3 others could use newly added tooltip support in {@link https://github.com/link CellLabelProvider}

@vogella https://github.com/vogella are you sure that also snippets like this should be removed?

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42100906 .

hendrikstill commented 10 years ago

You are right. I guess this code is more confusing than helpful for new users.

jeandersonbc commented 10 years ago

Snippet 023 was deleted from base code and from the wiki page (https://wiki.eclipse.org/JFaceSnippets)

vogella commented 10 years ago

Thanks!

2014-05-06 16:24 GMT+02:00 Jeanderson Barros Candido < notifications@github.com>:

Snippet 023 was deleted from base code and from the wiki page ( https://wiki.eclipse.org/JFaceSnippets)

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42308330 .

jeandersonbc commented 10 years ago

I just sent a patch for snippets 058 and 059. I think we are done with package org.eclipse.jface.snippets.dialogs

vogella commented 10 years ago

Cool. Just for priority reasons I suggest to first finish the Jface viewer snippets Am 06.05.2014 17:06 schrieb "Jeanderson Barros Candido" < notifications@github.com>:

I just sent a patch for snippets 058 and 059. I think we are done with package org.eclipse.jface.snippets.dialogs

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42314433 .

jeandersonbc commented 10 years ago

Many Snippets have a MyModel class to represent dummy data to be displayed. There are just few variations of it.

I guess we could move them to a separate class so we won't have the amount of repetitive code and the snippets should look more cleaner without them nested.

vogella commented 10 years ago

Snippets should be self contained , do a class reference is unfortunately not possible. Am 06.05.2014 18:34 schrieb "Jeanderson Barros Candido" < notifications@github.com>:

Many Snippets have a MyModel class to represent dummy data to be displayed. There are just few variations of it.

I guess we could move them to a separate class so we won't have the amount of repetitive code and the snippets should look more cleaner without them nested.

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42326039 .

jeandersonbc commented 10 years ago

Okay. I will just continue the general cleanup.

Thanks

vogella commented 10 years ago

Maybe align the data whenever possible Am 06.05.2014 18:37 schrieb "Jeanderson Barros Candido" < notifications@github.com>:

Okay. I will just continue the general cleanup.

Thanks

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42326434 .

jeandersonbc commented 10 years ago

Hello Lars. I finished some cleanings in org.eclipse.jface.snippets.{wizard, window}. Considering all previous cleanings, I guess we only have some more viewers to finish.

I'm waiting for you to review my pending patches and meanwhile I will check other cleanings from JFace code.

vogella commented 10 years ago

Great, in the last review I did, I noticed that lots of examples are still using Arrays as input. We should also change that to lists where ever applicable. Am 07.05.2014 05:36 schrieb "Jeanderson Barros Candido" < notifications@github.com>:

Hello Lars. I finished some cleanings in org.eclipse.jface.snippets.{wizard, window}. Considering all previous cleanings, I guess we only have some more viewers to finish.

I'm waiting for you to review my pending patches and meanwhile I will check other cleanings from JFace code.

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42386407 .

jeandersonbc commented 10 years ago

About the use of array, I tried to use lists as you suggested but data was not showing properly. Do you have something in mind @vogella ?

vogella commented 10 years ago

Maybe the input was set incorrectly. Can you send me an example in which it did not work? Am 07.05.2014 07:48 schrieb "Jeanderson Barros Candido" < notifications@github.com>:

About the use of array, I tried to use lists as you suggested but data was not showing properly. Do you have something in mind @vogellahttps://github.com/vogella?

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42391756 .

jeandersonbc commented 10 years ago

Case 1: ClassCastException

    private MyModel[] createModel() {
        List<MyModel> elements = new ArrayList<MyModel>(10);

        for (int i = 0; i < elements.size(); i++) {
            elements.set(i, new MyModel(i));
        }
        return (MyModel[]) elements.toArray();
    }

Case 2: Empty viewer

    public Snippet004HideSelection(Shell shell) {
        // code...
        v.setContentProvider(ArrayContentProvider.getInstance());
        List<MyModel> model = createModel();
        v.setInput(model);
        // code...
    }
    private List<MyModel> createModel() {
        List<MyModel> elements = new ArrayList<MyModel>(10);

        for (int i = 0; i < elements.size(); i++) {
            elements.set(i, new MyModel(i));
        }
        return elements;
    }

I think the second approach is something related to what you had in mind. However, the viewer shows with no data. By looking to this code, the problem seems to be the content provider since it is responsible for sending the content to the viewer. In addition, the viewer may not be able to handle Lists (v.setInput(model) but I my opinion I think the problem relies on the content provider.

vogella commented 10 years ago

Case 1: ClassCastException

private MyModel[] createModel() {
    List<MyModel> elements = new ArrayList<MyModel>(10);

    for (int i = 0; i < elements.size(); i++) {
        elements.set(i, new MyModel(i));
    }
    return (MyModel[]) elements.toArray();
}

This should be

private List createModel() { List elements = new ArrayList(10);

    for (int i = 0; i < elements.size(); i++) {
        elements.set(i, new MyModel(i));
    }
    return elements;

}

2014-05-07 18:25 GMT+02:00 Jeanderson Barros Candido < notifications@github.com>:

Case 1: ClassCastException

private MyModel[] createModel() {
    List<MyModel> elements = new ArrayList<MyModel>(10);

    for (int i = 0; i < elements.size(); i++) {
        elements.set(i, new MyModel(i));
    }
    return (MyModel[]) elements.toArray();
}

Case 2: Empty viewer

public Snippet004HideSelection(Shell shell) {
    // code...
    v.setContentProvider(ArrayContentProvider.getInstance());
    List<MyModel> model = createModel();
    v.setInput(model);
    // code...
}
private List<MyModel> createModel() {
    List<MyModel> elements = new ArrayList<MyModel>(10);

    for (int i = 0; i < elements.size(); i++) {
        elements.set(i, new MyModel(i));
    }
    return elements;
}

I think the second approach is something related to what you had in mind. However, the viewer shows with no data. By looking to this code, the problem seems to be the content provider since it is responsible for sending the content to the viewer. In addition, the viewer may not be able to handle Lists (v.setInput(model) but I my opinion I think the problem relies on the content provider.

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42449320 .

vogella commented 10 years ago

Case 2, works fine for me if you fill the list correctly. I update Snippet004HideSelection as a demo for you.

https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=b8868cd68c539df6b701562ebf63e91639ebb191

2014-05-07 18:25 GMT+02:00 Jeanderson Barros Candido < notifications@github.com>:

Case 1: ClassCastException

private MyModel[] createModel() {
    List<MyModel> elements = new ArrayList<MyModel>(10);

    for (int i = 0; i < elements.size(); i++) {
        elements.set(i, new MyModel(i));
    }
    return (MyModel[]) elements.toArray();
}

Case 2: Empty viewer

public Snippet004HideSelection(Shell shell) {
    // code...
    v.setContentProvider(ArrayContentProvider.getInstance());
    List<MyModel> model = createModel();
    v.setInput(model);
    // code...
}
private List<MyModel> createModel() {
    List<MyModel> elements = new ArrayList<MyModel>(10);

    for (int i = 0; i < elements.size(); i++) {
        elements.set(i, new MyModel(i));
    }
    return elements;
}

I think the second approach is something related to what you had in mind. However, the viewer shows with no data. By looking to this code, the problem seems to be the content provider since it is responsible for sending the content to the viewer. In addition, the viewer may not be able to handle Lists (v.setInput(model) but I my opinion I think the problem relies on the content provider.

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42449320 .

vogella commented 10 years ago

Sorry, final commit is https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d162216511eaf12560f9b48a8f226f39406ad1e3

2014-05-07 20:42 GMT+02:00 Lars Vogel lars.vogel@gmail.com:

Case 2, works fine for me if you fill the list correctly. I update Snippet004HideSelection as a demo for you.

https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=b8868cd68c539df6b701562ebf63e91639ebb191

2014-05-07 18:25 GMT+02:00 Jeanderson Barros Candido < notifications@github.com>:

Case 1: ClassCastException

private MyModel[] createModel() {
    List<MyModel> elements = new ArrayList<MyModel>(10);

    for (int i = 0; i < elements.size(); i++) {
        elements.set(i, new MyModel(i));
    }
    return (MyModel[]) elements.toArray();
}

Case 2: Empty viewer

public Snippet004HideSelection(Shell shell) {
    // code...
    v.setContentProvider(ArrayContentProvider.getInstance());
    List<MyModel> model = createModel();
    v.setInput(model);
    // code...
}
private List<MyModel> createModel() {
    List<MyModel> elements = new ArrayList<MyModel>(10);

    for (int i = 0; i < elements.size(); i++) {
        elements.set(i, new MyModel(i));
    }
    return elements;
}

I think the second approach is something related to what you had in mind. However, the viewer shows with no data. By looking to this code, the problem seems to be the content provider since it is responsible for sending the content to the viewer. In addition, the viewer may not be able to handle Lists (v.setInput(model) but I my opinion I think the problem relies on the content provider.

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42449320 .

jeandersonbc commented 10 years ago

Thanks Lars, I appreciate your assistance :+1:

By the way, the evil command was my attempt to not use magic numbers by using elements.size(). It turns out that besides saying the initial capacity, the real size was "0" therefore nothing to display.

Living and learning

vogella commented 10 years ago

:-) Am 07.05.2014 22:17 schrieb "Jeanderson Barros Candido" < notifications@github.com>:

Thanks Lars, I appreciate your assistance [image: :+1:]

By the way, the evil command was my attempt to not use magic numbers by using elements.size(). It turns out that besides saying the initial capacity, the real size was "0" therefore nothing to display.

Living and learning

— Reply to this email directly or view it on GitHubhttps://github.com/jeandersonbc/gsoc14-eclipse-planning/issues/5#issuecomment-42477775 .