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

[[ DocumentFilename ]] Add documentFilename property #3044

Merged
merged 6 commits into from Oct 19, 2015

Conversation

Projects
None yet
5 participants
@montegoulding
Copy link
Contributor

commented Oct 11, 2015

As discussed on the engine forums I have implemented a documentFilename property which sets the represented filename of the window on mac. I'm happy to look at other platforms if they support anything like this but I haven't found anything thus far.

@peter-b

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2015

@montegoulding N.b. Travis CI tests failed… could you please investigate?

@montegoulding

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2015

@peter-b Totally missed that window isn't a MCPlatformWindowRef on all platforms. I guess it's only Mac because it needed refactoring for Cocoa anyway and the others are waiting for someone to have time. So I guess add updatedocumentfilename to desktop-stack and all the platform stacks...

@@ -221,6 +224,9 @@ class MCPlatformWindow
bool m_use_text_input : 1;
bool m_is_realized : 1;
};

// MERG-2015-10-11: [[ DocumentFilename ]] documentFilename property
MCStringRef m_document_filename;

This comment has been minimized.

Copy link
@runrevmark

runrevmark Oct 12, 2015

Contributor

Could you move this to be below m_cursor? The layout of the struct is essentially:

internal stuff
changed flags
property values
object state

m_document_filename counts as a 'property value' :)


Syntax: set the documentFilename of <stack> to <filename>

Summary: Specifies the file path to the file that the stack is represents.

This comment has been minimized.

Copy link
@peter-b

peter-b Oct 12, 2015

Contributor

I think you mean "stack represents" rather than "stack is represents".

@@ -65,6 +65,9 @@ MCPlatformWindow::MCPlatformWindow(void)
m_is_realized = false;

m_is_opaque = true;

// MERG-2015-10-11: [[ DocumentFilename ]] documentFilename property
m_document_filename = nil;

This comment has been minimized.

Copy link
@peter-b

peter-b Oct 12, 2015

Contributor

Might it be better to initialise:

m_document_filename = MCValueRetain(kMCEmptyString);

and then require you could remove quite a lot of null checks for m_document_filename (i.e. the assumption would be that it must always be a valid MCStringRef)?

// MERG-2015-10-11: [[ DocumentFilename ]] Add stack documentFilename property
void MCStack::GetDocumentFilename(MCExecContext &ctxt, MCStringRef& r_document_filename)
{
if (m_document_filename == nil)

This comment has been minimized.

Copy link
@runrevmark

runrevmark Oct 12, 2015

Contributor

Initialize m_document_filename in the MCStack constructor to be MCValueRetain(kMCEmptyString) and this then simplifies this method.

If you just return, then 'r_document_filename' will be uninitialized and things will go wonky!

This comment has been minimized.

Copy link
@montegoulding

montegoulding Oct 12, 2015

Author Contributor

OK

{
MCStringRef t_resolved_filename;

if (!MCS_resolvepath(p_document_filename, t_resolved_filename))

This comment has been minimized.

Copy link
@runrevmark

runrevmark Oct 12, 2015

Contributor

You'll probably want to check whether p_document_filename is empty here before resolving.

This comment has been minimized.

Copy link
@montegoulding

montegoulding Oct 12, 2015

Author Contributor

Ah, yes I just noticed that it will now end up with the icon for the default folder... good point. Will fix

@montegoulding

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2015

I think I've resolved everything you have commented on in the last commit. One quirk I discovered is setRepresentedFilename chokes if you set to nil and none of the windows appear. I'm now checking for empty string before passing to the native path in case that at some point does something interesting to empty strings...

@livecodefraser

This comment has been minimized.

Copy link
Member

commented Oct 14, 2015

@livecode-vulcan

This comment has been minimized.

Copy link

commented Oct 14, 2015

💙 review by @livecodefraser ok f417361

livecode-vulcan added a commit that referenced this pull request Oct 14, 2015

Auto-merge pull request #3044 from montegoulding/feature/represented_…
…filename

[[ DocumentFilename ]] Add documentFilename property

As discussed on the engine forums I have implemented a documentFilename property which sets the represented filename of the window on mac. I'm happy to look at other platforms if they support anything like this but I haven't found anything thus far.
@livecode-vulcan

This comment has been minimized.

Copy link

commented Oct 14, 2015

😎 test success f417361

@peter-b

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

@montegoulding Please update to current develop. Thanks!

Merge branch 'develop' into feature/represented_filename
Conflicts:
	engine/src/executionerrors.h
@montegoulding

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2015

Ugh... looks like the IDE commit ref got messed up during the merge...

@montegoulding

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2015

Hopefully you are happy with that otherwise I'll need to squash it all on a new branch.

@runrevmark

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2015

@livecode-vulcan

This comment has been minimized.

Copy link

commented Oct 19, 2015

💙 review by @runrevmark ok 7bbffec

livecode-vulcan added a commit that referenced this pull request Oct 19, 2015

Auto-merge pull request #3044 from montegoulding/feature/represented_…
…filename

[[ DocumentFilename ]] Add documentFilename property

As discussed on the engine forums I have implemented a documentFilename property which sets the represented filename of the window on mac. I'm happy to look at other platforms if they support anything like this but I haven't found anything thus far.
@livecode-vulcan

This comment has been minimized.

Copy link

commented Oct 19, 2015

😎 test success 7bbffec

peter-b added a commit that referenced this pull request Oct 19, 2015

Merge pull request #3044 from montegoulding/feature/represented_filename
[[ DocumentFilename ]] Add documentFilename property

@peter-b peter-b merged commit 141febc into livecode:develop Oct 19, 2015

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/vulcan/cla Contributor Agreement signed by @montegoulding
continuous-integration/vulcan/pr The Vulcan build succeeded on 7 builders
continuous-integration/vulcan/review Approved by reviewer @runrevmark
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.