Adding support for DisplayModes for all .Net based templates and functions

[burningice2866]

DisplayModes is a native part of WebPages and MVC so its only natural that Composite C1 also supports it for its Functions and Templates.

This pullrequest adds supports for the default file-resolving exposed by DisplayModeProvider.GetAvailableDisplayModesForContext (https://msdn.microsoft.com/en-us/library/system.web.webpages.displaymodeprovider.getavailabledisplaymodesforcontext(v=vs.111).aspx) to dynamically choose a different file at execution time based on the given HttpRequest.

Existing Functions and Templates works exactly the same way, but its now possible to add a .mobile.cshtml or .tablet.ascx next to the existing file, and this will then be used to service a given request, instead of the normal one.

Read more about DisplayModes here

• https://www.simple-talk.com/dotnet/asp.net/multiple-views-and-displaymode-providers-in-asp.net-mvc-4/
• http://www.codeproject.com/Tips/704007/Desktop-and-Mobile-Browser-View-in-MVC

The code in this Pull Request has been part of the Community Build for about 2 years so its well tested and used in various of live production sites.

[VolodymyrMuzyka]

@burningice2866 great job, impressive as always!

P.S.Would you move CompositeC1Contrib to GitHub?

[mawtex]

Yay! Our first pull request on GitHub - thanks! Since this the first time we do this in a well functioning context (which CodePlex/TFS certainly wasn't) we try to learn a bit from this and expand the wiki on this topic.

We are bogged down with full day training sessions this whole week, so we won't be able to dive deep into the changes before next week, but below are some initial questions and comments.

• Comments – please use “/// <exclude />” if public methods, classes etc. should not show up in the public API. If they should show up, please add relevant comments.
• Can you give an example of when LegacyC1DisplayMode come in use? And why the work “legacy” is used?
• The keywords used for the query string are very generic and could end up creating unintended clashes. Prepending c1 or using c1device=... would solve that and align with other situations where we do something similar (c1mode for instance).
• The query string param evaluation create a situation where “print” and “mobile” will not work in conjunction with certain existing query strings. Examples are “?print&test” and “?test=1&print”. It would make sense to change this to a scheme where you can simply append something to the URL and it will work. The new v5 browser view feature allow you to append to the URL when doing a device view (like "Mobile"), so changing your evaluation to something that also works at the end of an existing URL would create a very nice fit here.

[burningice2866]

@aeont I'm usually more of a Mercurial guy, but I'm slowly getting the hang of Git so you might see C1Contrib here on GitHub before you know it :)

[burningice2866]

@mawtex thank you your feedback, it will be a quick task to decorate it all with

/// <exclude />

I agree that the QueryString (qs) overrides for Mobile and Desktop is up for discussion, and frankly I would go with any suggestion you might come with.

The reason for the LegacyC1DisplayMode, is because that the first versions of the implementation used "_" as separator, instead of "." which later became the default in WebPages. This code dates back more than 6 years, and the first public traces of it can be found here https://bitbucket.org/burningice/compositec1contrib/src/4b4df0e47040/Rendering.MasterPages/Web/MasterPageModule.cs?fileviewer=file-view-default#MasterPageModule.cs-65.

Two years later, Masterpages and RazorFunctions was no longer a part of C1Contrib and the feature was requested on Codeplex (http://compositec1.codeplex.com/workitem/1711). It didn't catch much attention unfortunately, and was therefor maintained in a custom build of C1 - the infamous "acto build".

[burningice2866]

I've removed the Print-mode, since sites that need such a mode are able to add them manually, using whatever querystring or other parameters of their choosing.

[burningice2866]

@mawtex so whats the deal here

• The print-mode has been removed since its easy for developers to add their own DisplayModeProviders
• I can go ahead and change the ?mobile querystring to ?c1displaymode=mobile and clean up the branch if that's the only thing holding back this PR

[mawtex]

Sorry for the laggy response. Questions and comments:

LegacyC1DisplayMode - I still have a problem understanding that name. Since you write it, I understand it makes complete sense to you, but for us and anyone else digging through our source code, what is this legacy thing? Is this a construct that exists to ensure backwards capability at your end? (why is this needed in our C1 core?).

SpecialModesFileResolver in Composite.Core.Application - is this the right namespace (Composite.Core.IO or Composite.AspNet feels more appropriate?) and is this a descriptive class name?

A quick read of the code give me the impression that a URL with ?mobile will trigger this feature - my plae for the use of a obscure word (c1device=...) was to ensure this feature is only triggered via this keyword and never ever inintended. Here my mode of thought is that I can imagine someone, sometime, somewhere make a feature where ?mobile is used, without actually wanting the rendering engine to change behavior. Right now both is supported?

[burningice2866]

@mawtex this is not new code written in October but is several years old and is used on a numerous sites. Its no different than you having to support .aspx extensions and media urls formatted as showmedia.ashx?id=... even though that's not the official way to render media urls.

In the same way, the official way to write a DisplayMode-file is <filename>.<DisplayMode>.<extensions>, but unfortunately the first commit (burningice2866@9c4f601#diff-c4161c4b012c40dcabf3d022135a5c7aR58) was implemented as <filename>_<DisplayMode>.<extensions>. This still has to be supported since there exists sites which uses _ instead of . as separator.

The same goes for ?desktop and ?mobile. I'm fine with using for c1device= going forward, but backward compatibility still has to be maintained. Or not...? I understand that there comes a time in every software projects life where you remove old API's, so that could be now for this feature if you insist.

Regarding the right namespace for the code, i'm fine with wherever it ends up since its not supposed to be used as a public api. Composite.Core.IO feels more right than Composite.AspNet though since it can be used for xml templates and xslt functions as well.

[mawtex]

If I understand you correctly, you suggest we pull in extra code/rules/complexity, so "official C1" is a better fit with the websites you have running on your branch of C1.

While I can see this would save you some time when upgrading those sites to run on our branch, this is not really a "nice clean commit". From our point of view we get classes, logic and rules which are kind of marked "deprecated" before we add it.

I will close this pull request without merging, but the core feature you are suggesting is super awesome and I would love if we can press "reset" and get this into the C1 core, in a way where we look at this as a "clean new feature".

[burningice2866]

I clearly said that i'm fine by removing old backward-compatibility code, so i don't understand why you feel the need to close the Pull Request. Give me 20 minutes and that code is gone.

[burningice2866]

Don't worry about "polluting the history". When all can agree on the final code, its normal practice to squash the commits into a single one which then gets merged. You can say a pull request is like pair programming... the code evolves by anyone giving their opinion and at the end its cleaned up and merged.

ie. http://programmers.stackexchange.com/questions/263164/why-squash-git-commits-for-pull-requests and https://github.com/ginatrapani/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit

[mawtex]

@burningice2866 the pull request is reopened

[burningice2866]

Thank you. I'll remove the last few backward-compatibility parts and change the namespace to
Composite.Core.IO and do a merge of your newest code since i see that there are some Merge Conflicts at the moment.

[mawtex]

Thanks for taking this time, appreciate it. My comments above was made after looking through half of the pull request, so rather than see it as a definitive list, see it as direction.

[burningice2866]

@mawtex pull request updated and is now green again for merging. As mentioned, before any merge the branch will be squashed, cleaned up and based out of your newest commit instead of the current 73b1096 from October last year.

[kasravi]

Can you undo changes related to spaces so changes could be easier to trace? there are lots of them

[burningice2866]

@kasravi if you're watching the diff via GitHub you can add w=1 as a querystring to the url, to ignore whitespace changes :)

https://github.com/Orckestra/C1-CMS/pull/4/files?w=1

Pretty neat.

[kasravi]

Thank for taking time to do this. There are things you should consider.

1. You shouldn’t change any indention, breaks and spaces in existing code, it unnecessarily complicates the code review. See this http://tirania.org/blog/archive/2010/Dec-31.html
2. You have changed type of template from an interface to object in TemplateDefinitionHelper. BindPlaceholders. It leads to lots of disambiguates in near future.
3. There is no need to be limited on two types of display modes. This shouldn’t be hardcoded.
4. The SpecialModesFileResolver doesn’t belong to IO namespace. Use of HTTP Context and that this is purely used by ASPNet related features, relates it to the ASPNet. Also, using “DisplayMode” in the class name would make more sense than SpecialMode.

[burningice2866]

1. I don't get why you're linking to that blog post. It talks about renaming methods and variables, restructuring code and refactoring into new files. Not once is whitespace or indentation mentioned in the blog post. Like i said earlier, you can use the w=1 querystring on GitHub to ignore whitespace, and if you're using a diff-viewer application you can ignore whitespaces as well. Following your reasoning, incorrect whitespacing can never be fixed because it causes confusion when watching diffs. Like this 466a176 or this 4a04525 or this 4c019af commit is wrong because it also fixes whitespace and indentation.
2. It needs to, since a different displaymode-template isn't necessarily a IPageTemplate anymore, but can be MasterPage or WebPageBase. And besides,TemplateDefinitionHelper.BindPlaceholders has no requirements towards what type its getting passed since its doing all its work via Reflection directly on the object type. I'm curious how you think it leads to lots of disambiguates in the future. TemplateDefinitionHelper.BindPlaceholders is a highly specialized method - infrastructure like one might say - that takes a range of very specific arguments, so its not like one ends up calling the method by accident or doesn't know what to pass to it, if one find himself in the situation where one needs to call it.
3. What makes you think there can be only two displaymodes? You can add a 1000 displaymodes if you want by adding them to DisplayModeProvider.Instance.Modes at Application Startup.
4. IO namespace was @mawtex suggestion. I've renamed to DisplayModesFileResolver and moved the class to Composite.AspNet namespace.

[mawtex]

Like i said earlier, you can use the w=1 query string on GitHub to ignore white space,

It is a lot easier to read on default diff viewers, when white space reformatting is avoided - so easier to read, by default - which I guess we all feel is a good thing. If you do not have a compelling reason to change white spaces, please don't. No need to fix it this time around, but please resist the urge to re-format (or - make pure "re-format" check-ins if we have something in dire need of it).

What makes you think there can be only two displaymodes? You can add a 1000 displaymodes if you want by adding them to DisplayModeProvider.Instance.Modes at Application Startup.

I think the point here was that your pull request only allows for two of those modes to be activated via the URL, while - as you say - there can be N. The alternative to "desktop/mobile only" is to come with a user agent string. Having a third option like "useragent;(user agent string)" could work and it would also work well with the device simulator in the C1 Console. Or can you see another way to 'open up' to more devices here?

I'm also thinking we should require a user to be logged in, when overriding c1displaymode. Any reason we would expose this publicly?

[kasravi]

HttpContext.SetOverriddenBrowser is setting a cookie that last for 7 days I think, if you test your code with emulator and then switch back to normal would see the bug it creates. its better to clear this cookie at session end. maybe HttpContext.ClearOverriddenBrowser() could help you.

Try to replace object type in Bindplaceholders with appropriate interface. yes it is easier and it works but it makes the code harder to maintain

[burningice2866]

The reason for exposing c1displaymode is mainly to

1. offer a "View in desktop" link to mobile devices, which is a very common request from customers.
2. quickly activate IsMobile to see how a mobile (not responsive) site looks on your desktop developing machine

@kasravi When using emulators you have no need to use c1displaymode, since you will present yourself as ie. an actual mobile device, so no issues there.

[mawtex]

We were primarily focusing on this feature in conjunction with the C1 Console browser's "device view" feature (the two really fit well together), but with a sticky cookie keeping browsers in the last selected mode I can see this make perfect sense as a public facing feature.

If there was a switch in the URL (like "c1displaymode=adhoc;mobile") which allow this to happen on a "per request" basis, this could work in the console as well. Nice to have, since this is beyond the feature you are offering in the pull request.