PATCH - SDRemote LO

classic Classic list List threaded Threaded
11 messages Options
juniorcesar juniorcesar
Reply | Threaded
Open this post in threaded view
|

PATCH - SDRemote LO

I declare that all of my past & future contributions to LibreOffice
may be licensed under
the MPL/LGPLv3+ dual license.

Hello, the patch is attached to the resolution of bug 61570 SDremote project. The patch has been created for the following academic UTFPR-Brazil: Junior Cesar de Oliveira, Ana Claudia Maciel, Willyan Schultz Dworak.


Junior.

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

sdremote.patch (54K) Download Attachment
Andrzej Hunt Andrzej Hunt
Reply | Threaded
Open this post in threaded view
|

Re: PATCH - SDRemote LO

Hi Junior,

Unfortunately your patch includes a lot of unnecessary reformatting
(space indentation replaced with tab-stops),
specifically in SlideShowActivity.java it's impossible to see what has
changed since the whole file has had all space-indentation replaced with
tab-indentation (i.e. git thinks the whole file has been removed and
recreated).

(There are also a few cases of lines with trailing space which the git
commit-hooks would usually complain about.)

No idea which editor/IDE you use, but if you could change back to space
indentation that would be hugely simplify reviewing the patch -- it
should probably be enough to configure it to use spaces for indentation
(four spaces per tab) and then reformat the file (I'm guessing you might
be using Eclipse?) which would remove most of the reformatting in the
patch.

Cheers,

Andrzej

On Tue, 2013-10-01 at 14:49 -0300, Junior Cesar Oliveira wrote:

> I declare that all of my past & future contributions to LibreOffice
> may be licensed under
> the MPL/LGPLv3+ dual license.
>
>
> Hello, the patch is attached to the resolution of bug 61570 SDremote
> project. The patch has been created for the following academic
> UTFPR-Brazil: Junior Cesar de Oliveira, Ana Claudia Maciel, Willyan
> Schultz Dworak.
>
>
>
>
>
> Junior.
> _______________________________________________
> LibreOffice mailing list
> [hidden email]
> http://lists.freedesktop.org/mailman/listinfo/libreoffice


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
juniorcesar juniorcesar
Reply | Threaded
Open this post in threaded view
|

Re: PATCH - SDRemote LO

Hi, we fixed the patch and send again.

Thanks.

Att Junior


2013/10/1 Andrzej Hunt <[hidden email]>
Hi Junior,

Unfortunately your patch includes a lot of unnecessary reformatting
(space indentation replaced with tab-stops),
specifically in SlideShowActivity.java it's impossible to see what has
changed since the whole file has had all space-indentation replaced with
tab-indentation (i.e. git thinks the whole file has been removed and
recreated).

(There are also a few cases of lines with trailing space which the git
commit-hooks would usually complain about.)

No idea which editor/IDE you use, but if you could change back to space
indentation that would be hugely simplify reviewing the patch -- it
should probably be enough to configure it to use spaces for indentation
(four spaces per tab) and then reformat the file (I'm guessing you might
be using Eclipse?) which would remove most of the reformatting in the
patch.

Cheers,

Andrzej

On Tue, 2013-10-01 at 14:49 -0300, Junior Cesar Oliveira wrote:
> I declare that all of my past & future contributions to LibreOffice
> may be licensed under
> the MPL/LGPLv3+ dual license.
>
>
> Hello, the patch is attached to the resolution of bug 61570 SDremote
> project. The patch has been created for the following academic
> UTFPR-Brazil: Junior Cesar de Oliveira, Ana Claudia Maciel, Willyan
> Schultz Dworak.
>
>
>
>
>
> Junior.
> _______________________________________________
> LibreOffice mailing list
> [hidden email]
> http://lists.freedesktop.org/mailman/listinfo/libreoffice




_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

sdremote.patch (7K) Download Attachment
Andrzej Hunt Andrzej Hunt
Reply | Threaded
Open this post in threaded view
|

Re: PATCH - SDRemote LO

Hi Junior,

Thanks for the updated patch, there were still some minor niggles though
which I had to fix (see bottom of email).


Currently your patch only changes the sound settings at the start of a
presentation, I'm not sure if we'd want to change back to the original
volume settings once the presentation is finished though?


The minor issues:

- android:defaultValue="true" was removed for two CheckBoxPreference
settings -- I assume this was unintentional (possibly an overly keen IDE
removing things automatically)?

- Some trailing spaces and mixes of tabs/spaces left over -- git
complains about both of these when applying patches:

Since you aren't doing a full build the git commit hooks aren't being
installed, meaning you aren't warned when there are issues with
spacing/tabs/formatting -- you can force installation of the hooks by
running "./g -z" in the LibreOffice tree. (However anyone with the
commit hooks in place, i.e. most LO devs, will have warnings shown which
prevent use of the commit until the issues are fixed -- which is what I
had to do to test the patch.)

(The cleaned up patch is attached.)

Cheers,

        Andrzej

On Thu, 2013-10-03 at 14:10 -0300, Junior Cesar Oliveira wrote:

> Hi, we fixed the patch and send again.
>
>
> Thanks.
>
>
> Att Junior
>
>
> 2013/10/1 Andrzej Hunt <[hidden email]>
>         Hi Junior,
>        
>         Unfortunately your patch includes a lot of unnecessary
>         reformatting
>         (space indentation replaced with tab-stops),
>         specifically in SlideShowActivity.java it's impossible to see
>         what has
>         changed since the whole file has had all space-indentation
>         replaced with
>         tab-indentation (i.e. git thinks the whole file has been
>         removed and
>         recreated).
>        
>         (There are also a few cases of lines with trailing space which
>         the git
>         commit-hooks would usually complain about.)
>        
>         No idea which editor/IDE you use, but if you could change back
>         to space
>         indentation that would be hugely simplify reviewing the patch
>         -- it
>         should probably be enough to configure it to use spaces for
>         indentation
>         (four spaces per tab) and then reformat the file (I'm guessing
>         you might
>         be using Eclipse?) which would remove most of the reformatting
>         in the
>         patch.
>        
>         Cheers,
>        
>         Andrzej
>        
>         On Tue, 2013-10-01 at 14:49 -0300, Junior Cesar Oliveira
>         wrote:
>         > I declare that all of my past & future contributions to
>         LibreOffice
>         > may be licensed under
>         > the MPL/LGPLv3+ dual license.
>         >
>         >
>         > Hello, the patch is attached to the resolution of bug 61570
>         SDremote
>         > project. The patch has been created for the following
>         academic
>         > UTFPR-Brazil: Junior Cesar de Oliveira, Ana Claudia Maciel,
>         Willyan
>         > Schultz Dworak.
>         >
>         >
>         >
>         >
>         >
>         > Junior.
>        
>         > _______________________________________________
>         > LibreOffice mailing list
>         > [hidden email]
>         > http://lists.freedesktop.org/mailman/listinfo/libreoffice
>        
>        
>        
>
>

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

0001-Adding-silent-mode-to-start-the-slideshow.patch (5K) Download Attachment
Artur Dryomov Artur Dryomov
Reply | Threaded
Open this post in threaded view
|

Re: PATCH - SDRemote LO

Hey guys,

This conversation somehow was moved to my “Trash” directory, probably it is time to check filters…

I’m a bit late, but this feature was already implemented a couple of days ago. Junior, thank you for your work and sorry I’m late with news ;-(

Thanks again!
Artur.
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Igor Steinmacher Igor Steinmacher
Reply | Threaded
Open this post in threaded view
|

Re: PATCH - SDRemote LO

Hi Artur,

did it occur because the bug report was outdated?

--
Igor Fabio Steinmacher
Visiting Scholar in Dept of Informatics at UCI (http://www.informatics.uci.edu/)
Faculty in Dept. of Computing at Universidade Tecnológica Federal do Paraná


On Sun, Oct 6, 2013 at 4:44 PM, Artur Dryomov <[hidden email]> wrote:
Hey guys,

This conversation somehow was moved to my “Trash” directory, probably it is time to check filters…

I’m a bit late, but this feature was already implemented a couple of days ago. Junior, thank you for your work and sorry I’m late with news ;-(

Thanks again!
Artur.
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Artur Dryomov Artur Dryomov
Reply | Threaded
Open this post in threaded view
|

Re: PATCH - SDRemote LO

Hi Igor,

I don’t think so. The next version of the Impress Remote for Android is not released yet, so I didn’t closed related bugs yet. Maybe it is better to close them when they are fixed at the master really, I just fell that users should have an ability to confirm the fix. We haven’t decided what version it will be as well so it is hard to track things.

Regards,
Artur.

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Andrzej Hunt Andrzej Hunt
Reply | Threaded
Open this post in threaded view
|

Re: PATCH - SDRemote LO

Hi Artur,

It's usually best to mark bugs as "Resolved" -- "Fixed" when the patch
is committed.

If you include the bug number in the form "fdo#12345678" in the commit
subject then bugzilla will automatically be updated to "Resolved-Fixed"
along with a message detailing when it'll be available in daily builds
etc -- i.e. there's no need to actually do anything in bugzilla if you
do this.

For example this commit:
http://cgit.freedesktop.org/libreoffice/core/commit/?id=bfd495d9754e293d6561363ab6e27b45e2e403f0
generated this automatic update to bugzilla:
https://bugs.freedesktop.org/show_bug.cgi?id=63035#c3

HTH,

Andrzej

On Mon, 2013-10-07 at 20:00 +0300, Artur Dryomov wrote:

> Hi Igor,
>
>
> I don’t think so. The next version of the Impress Remote for Android
> is not released yet, so I didn’t closed related bugs yet. Maybe it is
> better to close them when they are fixed at the master really, I just
> fell that users should have an ability to confirm the fix. We haven’t
> decided what version it will be as well so it is hard to track things.
>
>
> Regards,
> Artur.


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Miklos Vajna-4 Miklos Vajna-4
Reply | Threaded
Open this post in threaded view
|

Re: PATCH - SDRemote LO

On Mon, Oct 07, 2013 at 07:05:06PM +0100, Andrzej Hunt <[hidden email]> wrote:
> If you include the bug number in the form "fdo#12345678" in the commit
> subject then bugzilla will automatically be updated to "Resolved-Fixed"
> along with a message detailing when it'll be available in daily builds

Indeed a comment will be added, and target will be added to whiteboard,
but resolution won't be set automatically, as only the developer knows
if that single commit fixed the issue or more work is needed.

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

signature.asc (205 bytes) Download Attachment
Artur Dryomov Artur Dryomov
Reply | Threaded
Open this post in threaded view
|

Re: PATCH - SDRemote LO

Hi guys,

Yep, I know tricks about commit messages, thank you for the advise anyway ;-) I still think it’s better to mark bugs as resolved when the app will be released, let’s hope it will be done in near future.

I also CC Michael, maybe he has other points.

Regards,
Artur.


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Robinson Tryon Robinson Tryon
Reply | Threaded
Open this post in threaded view
|

Re: PATCH - SDRemote LO

On Tue, Oct 8, 2013 at 7:34 AM, Artur Dryomov <[hidden email]> wrote:
> Hi guys,
>
> Yep, I know tricks about commit messages, thank you for the advise anyway
> ;-) I still think it’s better to mark bugs as resolved when the app will be
> released, let’s hope it will be done in near future.

I haven't had a chance to try out one of the daily builds* myself, but
assuming that they are running smoothly, I'd encourage the same
workflow we use with bugs in LibreOffice itself (Test fix with daily
build and mark as RESOLVED FIXED when bug is no longer present).

From a QA standpoint, standardizing our treatment of LO and SDRemote
bugs is important to keeping the QA process as simple and
straightforward as possible :-)

As Miklos mentioned above, there is some automation that adds a
comment and target to a LibreOffice bug when a patch has been pushed
for it. This information is slightly less useful for SDRemote bugs, as
our release schedule for the remote is not in lockstep with our
LibreOffice releases, but the links to daily builds may indeed be
followed to find an SDRemote daily build*.

Cheers,
--R

* http://dev-builds.libreoffice.org/daily/master/Android-ARM@24-Bytemark-Hosting/current/
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice