Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-23493] Android: #append method in Ti.Filesystem.File #8513

Merged
merged 3 commits into from Nov 15, 2016

Conversation

frankieandone
Copy link
Contributor

@frankieandone frankieandone commented Oct 14, 2016

@@ -135,6 +134,19 @@ public boolean getWritable()
}

@Kroll.method
public boolean append(Object data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use tabs

return flagSymbolicLink;
}

public boolean append(Object data) throws IOException
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use tabs

@garymathews
Copy link
Contributor

It's best to not modify the original formatting of the files you are making changes to; it's hard to review as I cannot distinguish the original code from the changes you have made.

@sgtcoolguy
Copy link
Contributor

@fmerzadyan My comments from #8471 still apply. Appending using writers is incorrect and wasteful in terms of performance as well as may transform the contents. Please just copy bytes (for File) as you do with TiBlob.

Also, please try to avoid closing and re-opening new PRs for the same code/ticket. We lose history of the reviews and it becomes difficult to track between ticket and correct PR.

@sgtcoolguy
Copy link
Contributor

@fmerzadyan There's a merge conflict that needs to be resolved; looks like somehow one of Gary's commits made it into your branch; and lastly, we need to update the yaml docs to indicate that append is now supported on android (apidoc/Titanium/Filesystem/File.yml)

@frankieandone frankieandone force-pushed the timob-23493 branch 3 times, most recently from 47f0e1d to a7bb5a0 Compare November 3, 2016 20:51
@@ -95,6 +97,71 @@ public boolean isWriteable()
return file.canWrite();
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just make use of the write methods that already exist with an append flag?

@Override
public boolean append(Object data) throws IOException
{
    try {
        if (data instanceof String) {
            write((String) data, true);
        } else if (data instanceof TiFileProxy) {
            // write((TiFileProxy) data, true); // implement an append flag into this
        } else if (data instanceof TiBlob) {
            write((TiBlob) data, true);
        }
        return true;
    } catch (IOException e) {
        Log.e(TAG, "Error appending: ", e);
    }
    return false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're going that way then it would just be better to edit TiFileProxy instead because less changes would need to be made.

@@ -139,6 +138,13 @@ public boolean getWritable()
}

@Kroll.method
public boolean append(Object data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just do this, there's no need for any other changes.

@Kroll.method
public boolean append(Object data)
{
    return write(new Object[]{data, true});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was only 1 write method. The original write method had only one argument (Object[] args). I changed it so that it could take in append argument which is necessary. It keeps the original write exposed, the new write method not exposed and append is exposed too. But I'll be sure to change this one line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Titanium.Filesystem.write should already be exposed with the optional append parameter, so you can just make use of that 👍

{
try {

if (args != null && args.length > 0) {
boolean append = false;
if (args.length > 1 && args[1] instanceof Boolean) {
append = ((Boolean)args[1]).booleanValue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The args array contains the append Boolean, so there's no need to redefine it as a parameter. Which is why these changes aren't necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I removed that, my mistake.

@garymathews
Copy link
Contributor

CR: PASS
FR: PASS

👍

@garymathews
Copy link
Contributor

test this please

@garymathews garymathews merged commit a82de5b into tidev:master Nov 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants