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

SoSOL Metadata Preview #197

Closed
Edelweiss opened this issue Dec 30, 2016 · 40 comments
Closed

SoSOL Metadata Preview #197

Edelweiss opened this issue Dec 30, 2016 · 40 comments

Comments

@Edelweiss
Copy link

Display metadata that is specific to DCLP, such as ancient bibliography and information about layout and foliation. (ruby ka? xslt ka?)

@Edelweiss Edelweiss added the task label Dec 30, 2016
@Edelweiss Edelweiss self-assigned this Dec 30, 2016
@jcowey jcowey added this to the February 2017 milestone Jan 12, 2017
@paregorios paregorios assigned paregorios and unassigned Edelweiss Feb 15, 2017
@paregorios
Copy link
Member

Tom to do code review to determine how/where XSLT is invoked. Email thread with subject line "XSLT".

@jcowey
Copy link
Contributor

jcowey commented Feb 16, 2017

Dealing with a similar problem #199 is also important.

@Edelweiss
Copy link
Author

Am I understanding this correctly? You want the current table view to be replaced with an XSLT preview similar to the one you see under »text>preview«, just that the focus will be on metadata and that the text will omitted.

Current display:

bildschirmfoto 2017-02-22 um 10 18 02

@rla2118
Copy link

rla2118 commented Feb 22, 2017 via email

@paregorios
Copy link
Member

If I may, I think the issues are still tangled here. These seem to me to be the options:

  1. Replace the current, bespoke "table" mechanism for displaying metadata in the editor preview with something that exploits the metadata portions of the XSLT used in the navigator.
  2. Keep the current, bespoke "table" mechanism for displaying metadata in the editor preview and commit to updating/modifying/maintaining it separately to keep it sufficiently in line with what the navigator XSLT does.

My recommendation is number 1. Otherwise we are wasting time maintaining multiple sets of code with parallel function.

@Edelweiss if there is a technical problem with implementing a solution like no. 1, please say so here. Thanks.

@Edelweiss
Copy link
Author

I don’t assume that there should be any tech problems with solution #1. I will take a look at the implementation for ddb files to find out how best to deal with this issue.

@Edelweiss
Copy link
Author

Edelweiss commented Mar 30, 2017

I came up with a solution that seems to be doing the job. The code looks somewhat cluttered and hacked, though. For instance, I’m not sure wether we really need to call the biblio template (see lines 15-16, 20 and 24-35). And what about the path variables in lines 12-14? What are they needed for? Do they need to point to a specific location? I would be happy if someone like @paregorios, @leoba or @wsalesky could review the code to find out whether it’s unnecessarily complicated (or just not complicated enough?) or perhaps even altogether wrong.

@paregorios
Copy link
Member

XSLT code review needed on a new Papyrological Editor metadata preview stylesheet, which leverages our existing XSLT in order to provide on-demand previews of texts being edited in the editor. @Edelweiss indicates her concerns in the preceding comment. Would wone of @wsalesky @leoba @sarcanon be in a position to self-assign this ticket and undertake this review in the next few days?

@paregorios
Copy link
Member

@hcayless and @ryanfb can you guys comment on @m-k-r's note preceding, and suggest best approach?

@ryanfb
Copy link
Collaborator

ryanfb commented Aug 8, 2017

It looks like on the DCLP development branch, externals.yml has been updated to point at DCLP XSLT: https://github.com/DCLP/sosol/blob/development/config/externals.yml

@m-k-r
Copy link
Collaborator

m-k-r commented Aug 10, 2017

navigator and epidoc point now to the correct version.
DCLP/sosol@f2dc010
But still you have to copy epidoc into navigator/pn-sync.

@paregorios
Copy link
Member

@hcayless this is the issue that @m-k-r was raising during our skype call this morning. I think I actually muddied the waters with my attempted clarification of same.

@m-k-r now that I see this, I don't think you need to open a new ticket as @jcowey and I requested. Instead we just need to follow up your questions here.

@ryanfb when you are back I'd like to talk to you about this.

@paregorios paregorios assigned paregorios and unassigned wsalesky Aug 16, 2017
@paregorios
Copy link
Member

I think what needs doing here (@ryanfb please confirm/expand/correct) is making sure that the correct epidoc-xslt gets checked out during the setup along with the navigator xslt and that it's all in the right place.

@m-k-r
Copy link
Collaborator

m-k-r commented Aug 16, 2017

To clarify the problem:

https://github.com/DCLP/sosol/blob/development/script/setup#L19 creates the external dependencies which are defined in https://github.com/DCLP/sosol/blob/development/config/externals.yml.

These files are already configured for dclp.
The papyri navigator for sosol contains in pn-xslt the content of epidoc-xslt as can be seen here:
https://github.com/papyri/navigator/tree/c03dd1a2a21ca2cfe72eb0b2cc4306a02db5b4fa/pn-xslt
for comparison this is the content of the dclp navigator for sosol:
https://github.com/DCLP/navigator/tree/47f10946b633f1e253fb2bfc9ba109463d580aae/pn-xslt

So the editor needs in addition to the epidoc-xslt which is defined in externals.yml the epidoc-xslt content in navigator/pn-xslt.

For this reason we can't get a functioning editor with the setup-script because we have no version of the navigator which contains the content of epidoc-xslt in the pn-xslt directory.

My question was how we solve this.

The possibillities I see are:

  1. we simply copy epidoc-xslt in pn-xslt.
  2. we create regularly commits for the editor where we copy epidoc-xslt and afterwards delete it again
  3. we create a special branch for sosol-navigator which contains epidoc-xslt and update it regularly from the master branch. We could also strip it down to what sosol actually requires.
  4. we make changes to sosol and/or the navigator so it uses the epidoc-xslt-files from the epidoc-xslt in shared/externals.

@ryanfb
Copy link
Collaborator

ryanfb commented Aug 17, 2017

This is what the current version of papyri/navigator pn-xslt looks like (your link points to a very old commit): https://github.com/papyri/navigator/tree/master/pn-xslt (it looks very much like the current DCLP pn-xslt directory)

It defines an epidoc-xslt checkout to a sibling directory inside its Rakefile: https://github.com/papyri/navigator/blob/master/pn-xslt/Rakefile

The DCLP version of this is also now configured to point to the DCLP/epidoc-xslt repo: https://github.com/DCLP/navigator/blob/master/pn-xslt/Rakefile

As long as the navigator/pn-xslt Rakefile and SoSOL externals.yml point to the same repo+revision for their XSLT, and their respective checkout commands are actually used, they will have the same XSLT. The editor should/does not need the navigator-specific XSLT in pn-xslt.

@paregorios
Copy link
Member

paregorios commented Aug 17, 2017 via email

@m-k-r
Copy link
Collaborator

m-k-r commented Aug 18, 2017

@ryanfb I see a different behaviour.

I updated the externals.yml today so it points to our newest versions.

I ran "bundle exec cap local externals:setup" and after that "rake setup" in naviagator-root/pn-xslt/
https://github.com/DCLP/navigator/blob/master/pn-xslt/Rakefile
I got a blank page for "Commentary" until I did "rsync -avP epidoc-xslt/* pn-xslt/" inside the navigator root-directory.

Am I missing something?

The linked papyri-navigator is old but the version referenced here:
https://github.com/sosol/sosol/blob/master/config/externals.yml

ryanfb added a commit to ryanfb/sosol that referenced this issue Aug 18, 2017
@ryanfb
Copy link
Collaborator

ryanfb commented Aug 18, 2017

For "Commentary" you need to update https://github.com/DCLP/sosol/blob/development/data/xslt/ddb/commentary.xsl to point to the appropriate files. See: DCLP/sosol#2

m-k-r added a commit to DCLP/sosol that referenced this issue Aug 18, 2017
@m-k-r
Copy link
Collaborator

m-k-r commented Aug 18, 2017

Thanks, that worked.
I think we can close this issue.

@paregorios
Copy link
Member

paregorios commented Aug 18, 2017 via email

@m-k-r
Copy link
Collaborator

m-k-r commented Aug 18, 2017

The changes are merged to development.

@paregorios
Copy link
Member

@m-k-r if these changes are complete, tested, and working, then they should be merged to DCLP master. It would be ok with me if you want to do the merge yourself and then push the branch to github, or if you'd rather issue a pull request.

cc @jcowey

@paregorios paregorios assigned m-k-r and unassigned ryanfb and paregorios Aug 22, 2017
@m-k-r
Copy link
Collaborator

m-k-r commented Aug 23, 2017

I merged the changes to master

@paregorios
Copy link
Member

@ryanfb does this look to be resolved to you?

@ryanfb
Copy link
Collaborator

ryanfb commented Aug 23, 2017

Yes.

@ryanfb ryanfb closed this as completed Aug 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants